-
Notifications
You must be signed in to change notification settings - Fork 137
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
Add max limit of 2 groups to import blocks #566
Conversation
Codecov Report
@@ Coverage Diff @@
## main #566 +/- ##
=======================================
Coverage 34.76% 34.76%
=======================================
Files 44 44
Lines 3348 3348
=======================================
Hits 1164 1164
Misses 2085 2085
Partials 99 99
Continue to review full report at Codecov.
|
1bbd307
to
94073ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Manuel Mendez <[email protected]>
There isn't a whole lot of direction from the go project or tools for this. A quick google search did turn up 2 projects that also document 2 groups, both [Uber] and [CockroachDB]. We do have both goimports and gofumpt splitting stdlib vs others out if they are mixed. But they don't do any futher kind of grouping. So in the absence of concrete handling lets just settle on 2 because its easy to enforce. The script below deletes newlines within `import ( ... )` blocks and then runs gofumpt which will split and sort 2 at most 2 groups. [Uber]: https://github.com/uber-go/guide/blob/22f61e283ac2f75ba34b62f565581cc70e35fb7a/style.md#import-group-ordering [CockroachDB]: https://web.archive.org/web/20210509102622/https://wiki.crdb.io/wiki/spaces/CRDB/pages/181371303/Go+Golang+coding+guidelines#Import-Group-Ordering Signed-off-by: Manuel Mendez <[email protected]>
94073ce
to
d05bf69
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. Do we want to centralize this into the lint-install linters?
Good question @nshalman ! Just this PR or the whole file? (I think latter tbh). |
Description
Updates CONTRIBUTING.md with a Go style item.
Also enforces the new style via code in ./ci-checks.sh
Why is this needed
Consistency within the code base.
How Has This Been Tested?
Ran locally.
How are existing users impacted? What migration steps/scripts do we need?
NA
Checklist:
I have: