Automatically format imports #151
Conversation
Signed-off-by: Pavol Loffay <[email protected]>
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #151 +/- ##
=======================================
Coverage 96.21% 96.21%
=======================================
Files 28 28
Lines 1267 1267
=======================================
Hits 1219 1219
Misses 37 37
Partials 11 11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pavolloffay)
scripts/import-order-cleanup.py, line 3 at r1 (raw file):
import argparse def cleanup_imports_and_return(imports):
Isn't goimports sufficient? That's what my IDE is configured to use. Probably something like this would work:
$ goimports -w $(go list ./cmd/... ./pkg/...)
In the Makefile, the list of packages is stored in a var already.
scripts/import-order-cleanup.sh, line 5 at r1 (raw file):
set -e python scripts/import-order-cleanup.py -o $1 -t $(git ls-files "*\.go" | grep -v -e vendor)
If we do end up needing the script, it would probably be better suited for the .travis directory, as we don't have a scripts yet. The check target should also be changed, to call this instead of go fmt.
I will put it wherever you think it's the best. Seems odd to put things into
From https://godoc.org/golang.org/x/tools/cmd/goimports In addition to fixing imports, goimports also formats your code in the same style as gofmt so it can be used as a replacement for your editor's gofmt-on-save hook. |
Signed-off-by: Pavol Loffay <[email protected]>
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pavolloffay)
a discussion (no related file):
I will put it wherever you think it's the best. Seems odd to put things into .travis which are not related to travis...
If the script is called as part of make check, then it's checked by Travis ;-)
I ran the script locally and compared with goimports ..., and it does look like it does more things.
|
@jpkrohling is there anything missing in this PR? I would like to be consistent with jaeger repo to provide the same source code structure. Enforcing style at build time is the only way how to check the code is properly formatted. As you can see the imports are mismatched in the current master. |
There was a problem hiding this comment.
The only thing missing is to fail the build if a call to import-order-cleanup.sh yields changes.
Currently, the check target relies on the output of go fmt (not make format) to check if formatting changes need to be done and fail the CI build accordingly. With the changes from this PR, we need to add this script to the check as well, to also fail the build if changes were detected. You can take a look at the ensure-generate-is-noop for a way to check if the git clone isn't clean.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pavolloffay)
Signed-off-by: Pavol Loffay <[email protected]>
|
Check added |
Following the same approach as in
/jaegerrepoSigned-off-by: Pavol Loffay [email protected]