opentelemetry-collector icon indicating copy to clipboard operation
opentelemetry-collector copied to clipboard

Move implementation checks to test files

Open atoulme opened this issue 1 year ago • 4 comments

We use implementation checks to make sure an interface is fulfilled by a struct. However, those checks are sometimes made in the code we ship, which introduces dependencies in the code base. Instead, consider moving those to test files.

Additionally, we should add this as a best practice.

atoulme avatar Jan 30 '24 17:01 atoulme

I don't think this is a useful exercise. Noop type checks are removed by the compiler, so there's no runtime impact. But when you browse the code it's much more useful to see implemented interfaces declared next to the struct, not in some other place.

yurishkuro avatar Jan 30 '24 17:01 yurishkuro

Initial context is from https://github.com/open-telemetry/opentelemetry-collector/pull/9427#pullrequestreview-1851884365

Not necessary impacting this, but would like all the implementation checks to be moved to tests (see https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configopaque/opaque.go#L17) so that the linker does unnecessary bring that dependency to non-test code if unnecessary. In this case is a system func so not that worry, but want to set a pattern.

atoulme avatar Jan 30 '24 17:01 atoulme

If a type implements an interface, it's highly unlikely that it will be used in the context where that interface is not already linked, so the argument seems pretty weak to me.

yurishkuro avatar Jan 30 '24 19:01 yurishkuro

I have added the discussion-needed label and will bring it to a SIG meeting. I don't want to do work needlessly.

atoulme avatar Apr 11 '24 20:04 atoulme