-
Notifications
You must be signed in to change notification settings - Fork 14
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
Setup go-imports-organizer and organize Go imports #357
base: main
Are you sure you want to change the base?
Conversation
06e9d99
to
635f7c6
Compare
635f7c6
to
c594a88
Compare
c594a88
to
2e3452f
Compare
2e3452f
to
e4fe3dc
Compare
2bcceaf
to
608057a
Compare
608057a
to
9ee56c6
Compare
fmt: goimports ## Format the code using goimports | ||
find . -not -path '*/\.*' -name '*.go' -exec $(GOIMPORTS) -w {} \; | ||
fmt: ## Run go fmt verifications | ||
hack/verify-gofmt.sh |
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.
I used to run make fmt
to have the code formatted, but with this, it would just verify and exit, which is kinda surprising to me. If we are just calling ./hack/verify-gofmt.sh
, I would suggest renaming this Make target into verify-fmt
to make the intent clearer..
I noticed that goio
also formats the code as well, besides organizing the imports, right? So maybe this target could just be this instead:
fmt: imports ## Format the code using goio
Then a separate verify-fmt
target that would run hack/verify-gofmt.sh
.
WDYT?
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.
Do we need fmt verification at all?
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.
I agree fmt verification might not be necessary.
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.
@coreydaley WDYT?
Postpone for 1.3 |
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.
@coreydaley Could you please rebase your branch and fix the conflicts?
fmt: goimports ## Format the code using goimports | ||
find . -not -path '*/\.*' -name '*.go' -exec $(GOIMPORTS) -w {} \; | ||
fmt: ## Run go fmt verifications | ||
hack/verify-gofmt.sh |
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.
@coreydaley WDYT?
9ee56c6
to
18651c2
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@rm3l This pull request has just been rebased. |
18651c2
to
15e1b93
Compare
15e1b93
to
da6432f
Compare
Quality Gate passedIssues Measures |
This is what it would look like to implement and run the go-imports-organizer on this project. Please note that I have setup the dependencies to be vendored to support not having each user install the go-imports-organizer as a standalone tool