-
Notifications
You must be signed in to change notification settings - Fork 373
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
Correct import order #321
Correct import order #321
Conversation
Codecov Report
@@ Coverage Diff @@
## master #321 +/- ##
======================================
Coverage 57.3% 57.3%
======================================
Files 19 19
Lines 904 904
======================================
Hits 518 518
Misses 335 335
Partials 51 51
Continue to review full report at Codecov.
|
Hey thanks for bringing this up! This is indeed an annoying loophole in the current rules. Let's merge this now and evaluate impi in a separate PR. Do you happen to know the golangci-lint issue which tracks this bug? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: corneliusweig, ferhatelmas The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This might be because I did some refactoring in Goland IDE, which does a surprisingly poor job organizing |
Yes, goland is not great at this. But ultimately, it should be the job of goimports to set it right. My annoying workaround is currently to put all imports into one single block and then run goimports. |
Actually, it's not a golangci-lint issue but goimports. Canonical issue: golang/go#20818 Also there are various complaints in golangci-lint but there are mostly irrelevant, not being the root cause. That's why I didn't link them, here. impi is simple since it doesn't format but only validates. When we keep imports consistent and get alerted on wrong order, fixing should be easy then sounds good enough to me. Green light for impi integration? |
I'd say yes, let's try this out! |
https://github.com/kubernetes-sigs/krew/blob/master/.golangci.yml#L6 wasn't applied correctly.
This PR fixes existing issues but the root cause is still there. Since golangci-lint won't be fixed soon. I suggest integrating impi in the short run (as a follow up PR or update on this)