Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move implementation checks to test files #9431

Open
atoulme opened this issue Jan 30, 2024 · 4 comments
Open

Move implementation checks to test files #9431

atoulme opened this issue Jan 30, 2024 · 4 comments
Labels
discussion-needed Community discussion needed

Comments

@atoulme
Copy link
Contributor

atoulme commented Jan 30, 2024

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.

@yurishkuro
Copy link
Member

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.

@atoulme
Copy link
Contributor Author

atoulme commented Jan 30, 2024

Initial context is from #9427 (review)

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.

codeboten pushed a commit that referenced this issue Jan 30, 2024
@yurishkuro
Copy link
Member

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.

bogdandrutu pushed a commit that referenced this issue Jan 30, 2024
**Description:**
 move implementation checks to test files
**Link to tracking Issue:** 
#9431
@atoulme atoulme added the discussion-needed Community discussion needed label Apr 11, 2024
@atoulme
Copy link
Contributor Author

atoulme commented Apr 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion-needed Community discussion needed
Projects
None yet
Development

No branches or pull requests

2 participants