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

Fix vanity import #2277

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Oct 6, 2021

Follow up to #2197.

porto doesn't complain about this for some reason.

I don't think this warrants a Changelog entry, but I can't add the Skip Changelog label

For some reason `porto` doesn't work here?
@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #2277 (ac86a48) into main (7461a8d) will increase coverage by 72.5%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           main   #2277      +/-   ##
=======================================
+ Coverage      0   72.5%   +72.5%     
=======================================
  Files         0     168     +168     
  Lines         0   11874   +11874     
=======================================
+ Hits          0    8611    +8611     
- Misses        0    3029    +3029     
- Partials      0     234     +234     
Impacted Files Coverage Δ
internal/metric/registry/registry.go 86.8% <ø> (ø)
exporters/stdout/stdouttrace/config.go 100.0% <0.0%> (ø)
baggage/baggage.go 98.2% <0.0%> (ø)
exporters/otlp/otlptrace/exporter.go 0.0% <0.0%> (ø)
...ers/otlp/otlpmetric/internal/otlpconfig/options.go 65.8% <0.0%> (ø)
bridge/opencensus/exporter.go 100.0% <0.0%> (ø)
sdk/metric/controller/controllertest/test.go 100.0% <0.0%> (ø)
exporters/stdout/stdoutmetric/config.go 85.7% <0.0%> (ø)
propagation/baggage.go 100.0% <0.0%> (ø)
sdk/trace/id_generator.go 100.0% <0.0%> (ø)
... and 159 more

@Aneurysm9 Aneurysm9 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 6, 2021
@Aneurysm9
Copy link
Member

Trivial change to an import comment, merging with a single approval.

@Aneurysm9 Aneurysm9 merged commit 3d4ae8d into open-telemetry:main Oct 6, 2021
@mx-psi mx-psi deleted the mx-psi/fix-import-comment branch October 6, 2021 14:04
@jcchavezs
Copy link
Contributor

Interesting one @mx-psi. I think the reason why porto does not complain about this is because this is an internal package and porto ignore internal packages https://github.com/jcchavezs/porto/blob/aab52daf3ac470aa9fd85614d560dc157e0e8234/files.go#L26. I think internal packages should not have vanity imports because they aren't exported but I do agree in large code bases it is better to have it so maybe open an issue in porto to support internal packages through a flag?

@mx-psi
Copy link
Member Author

mx-psi commented Oct 7, 2021

I think the reason why porto does not complain about this is because this is an internal package and porto ignore internal packages

That makes sense, thanks for the explanation :)

I think internal packages should not have vanity imports because they aren't exported but I do agree in large code bases it is better to have it

I don't have a strong stance on whether they should/should not have the vanity imports; the problem is that if they have one and it's wrong it will break some builds (e.g. gobind and gomobile will complain). I opened jcchavezs/porto#11 to add a flag to check internal packages and we can add them everywhere if maintainers think it makes sense in this codebase (see #2279 for discussion).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants