presubmit: check import path ordering #3636
Conversation
Add a test for this as well as it's annoying to point out in every code review. Fix all the import paths that are flagged by this new test. Fixes: #3634 Signed-off-by: Miek Gieben <miek@miek.nl>
miekg
requested review from
bradbeam,
chrisohaver,
dilyevsky,
jameshartig,
grobie,
Isolus,
johnbelamaric,
nitisht,
pmoroney,
rajansandeep,
stp-ip,
SuperQ and
yongtang
as code owners
Codecov Report
@@ Coverage Diff @@
## master #3636 +/- ##
==========================================
- Coverage 56.6% 56.58% -0.03%
==========================================
Files 220 220
Lines 11039 11039
==========================================
- Hits 6249 6246 -3
- Misses 4311 4313 +2
- Partials 479 480 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Have we already looked at something like https://github.com/golangci/golangci-lint?
There's an option around checking for the code to be formatted, I can't remember off the top of my head if it check import path ordering.
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] presubmit: ch..." ]
bradbeam approved this pull request.
Have we already looked at something like https://github.com/golangci/golangci-lint?
There's an option around checking for the code to be formatted, I can't remember off the top of my head if it check import path ordering.
it doesn't check this particular case. But having something that nits about non-fmt-ed go
code is *also* welcome as I'm also tired of pointing that out in PRs.
|
|
See https://twitter.com/peterbourgon/status/1222512198972514305 new goimport will do this differently, and just lump them together. I'll update to that version and check, then we can drop this and just check gofmted-ness of the code. |
|
I can't replicate the new behaviour of goimports |
|
It seems like the I can confirm with some light testing that it puts them after but according to golang/go#31646 it puts them before? Either way, it doesn't seem to reorganize existing blocks. |
|
[ Quoting <notifications@github.com> in "Re: [coredns/coredns] presubmit: ch..." ]
It seems like the `-local` option might be what we want here but the docs say it puts local imports *after* the third-party imports. What do you think about that?
that would be annoying; then every contributor requires to add that flag.... Right now
goimports leaves them alone *if* you format them as required.
I can confirm with some light testing that it puts them after but according to golang/go#31646 it puts them before?
if goimports (with no flag) would do something sensible I'm fine with that, but that's not
that case right now...
|
|
Once goimports is changes/fixed we should probably revert this and stop caring about having three blocks. |
Add a test for this as well as it's annoying to point out in every code review. Fix all the import paths that are flagged by this new test. Fixes: coredns#3634 Signed-off-by: Miek Gieben <miek@miek.nl>
Add a test for this as well as it's annoying to point out in every code
review.
Fix all the import paths that are flagged by this new test.
Fixes: #3634