formatting: format code and imports with gci #2045
Conversation
iurii-ssv
requested review from
nkryuchkov,
moshe-blox and
anatolie-ssv
and removed request for
nkryuchkov,
moshe-blox and
anatolie-ssv
Makefile
Outdated
| .PHONY: format | ||
| format: | ||
| # both format commands must ignore generated files which are named *mock* or enr_fork_id_encoding.go | ||
| # the argument to gopls format can be a list of files | ||
| find . -name "*.go" ! -path '*mock*' ! -name 'enr_fork_id_encoding.go' -type f -print0 | xargs -0 -P 1 sh -c 'gopls -v format -w $0' | ||
| # the argument to gopls imports must be a single file so this entire command takes a few mintues to run | ||
| find . -name "*.go" ! -path '*mock*' ! -name 'enr_fork_id_encoding.go' -type f -print0 | xargs -0 -P 10 -I{} sh -c 'gopls -v imports -w "{}"' | ||
| gci write -s standard -s default -s blank -s dot -s localmodule -s "prefix(github.com/ssvlabs/ssv-spec)" $$(find . -type f -name '*.go' -not -path "*mock*" -not -name "*.gen.go") |
There was a problem hiding this comment.
Note, this PR doesn't change enr_fork_id_encoding.go file - so I don't think we need to add it here as "an exception" (and similar holds true for SSZ-generated encodings for example),
but in the future ideally we'd want to add .gen.go suffix for every generated file so it's taken care of by this new make format automatically
There was a problem hiding this comment.
update 1: actually we don't even need .gen.go suffix(es) - we can just use --skip-generated and it's smart enough to figure out what files are generated by analysing code-comments I guess
update 2: manually-written mock(s) should be formatted (only generated once should be skipped), so now adjusted accordingly
Makefile
Outdated
| find . -name "*.go" ! -path '*mock*' ! -name 'enr_fork_id_encoding.go' -type f -print0 | xargs -0 -P 1 sh -c 'gopls -v format -w $0' | ||
| # the argument to gopls imports must be a single file so this entire command takes a few mintues to run | ||
| find . -name "*.go" ! -path '*mock*' ! -name 'enr_fork_id_encoding.go' -type f -print0 | xargs -0 -P 10 -I{} sh -c 'gopls -v imports -w "{}"' | ||
| gci write -s standard -s default -s blank -s dot -s localmodule -s "prefix(github.com/ssvlabs/ssv-spec)" $$(find . -type f -name '*.go' -not -path "*mock*" -not -name "*.gen.go") |
There was a problem hiding this comment.
Why do we need a separate block for spec? I agree it's close to the ssv node, but we also have other dependencies in ssvlabs, like eth2-key-manager (or ssv-signer in the future). I think it should belong to 3rd party dependencies
There was a problem hiding this comment.
Does gci perform go fmt-like formatting as well?
There was a problem hiding this comment.
Why do we need a separate block for spec? I agree it's close to the ssv node, but we also have other dependencies in ssvlabs, like eth2-key-manager (or ssv-signer in the future). I think it should belong to 3rd party dependencies
hmm, maybe we don't - it's just a common dependency I thought "worth highlighting"
ssv-signer and eth2-key-manager usage is very limited while ssv-spec is common and it would be nice-to-spot right away
Does gci perform go fmt-like formatting as well?
it doesn't mention it anywhere, but as far as I can tell it does some basic code-formatting (such as removing extra spaces and so on) - I'm not sure if it's exactly matching gofmt / goimports in that sense though
I've been using it with previous projects in the past and haven't had any code-formatting issues - I guess if we discover any we can maybe add gofmt on top ?
There was a problem hiding this comment.
I've been using it with previous projects in the past and haven't had any code-formatting issues - I guess if we discover any we can maybe add gofmt on top ?
Sounds good
There was a problem hiding this comment.
hmm, maybe we don't - it's just a common dependency I thought "worth highlighting"
I agree it's the most common and close dependency but as far as I know there's no common convention for another block for such repos. Also, moving it to 3rd party block should reduce the number of conflicts by a lot because it's how we are generally using it now
There was a problem hiding this comment.
I don't mind grouping it with other 3rd-party dependancies,
but lets see if @moshe-blox @y0sher @MatusKysel have preferences one way or the other (and I'll update it accordingly)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files☔ View full report in Codecov by Sentry. |
|
Can we also add a linter check that all files are |
iurii-ssv
force-pushed
the
make-format-with-gci
branch
from
8c02268 to
b6ad35b
Compare
…r-check for gci + do not fix linter-discovered issues by default
Yeah, that's a good idea, added now |
| # Fix found issues (if it's supported by the linter) | ||
| fix: true | ||
| fix: false |
There was a problem hiding this comment.
I believe linter should be a read-only type of thing
|
I think it could be a good idea to have a CI job that checks if running |
Doesn't our linter check for this already (and it runs in a github pipeline) ? I think this is the linter setting responsible for checking against Lines 173 to 175 in 041be28 
|
This PR:
make formatcommand to use https://github.com/daixiang0/gci (it's fast and simple) for code & imports formatting, it is recommended to run it for every PR before we merge it (make lintwill fail otherwise)make formatto group & sort imports according to the following order: golang-pkgs -> 3rd-party-pkgs -> ssv-spec-pkgs -> local-ssv-pkgs