-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Pass only Git-tracked Go files to gofumpt #4809
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
Conversation
make format
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.
Seems fine to me (didn't test it though). I don't personally care very much, because I never format files manually; I rely on VS Code to do it automatically on save.
CONTRIBUTING.md
Outdated
@@ -109,7 +109,7 @@ by setting [`formatting.gofumpt`](https://github.com/golang/tools/blob/master/go | |||
To run gofumpt from your terminal go: | |||
|
|||
``` | |||
go install mvdan.cc/gofumpt@latest && gofumpt -l -w . | |||
go install mvdan.cc/gofumpt@latest && make format |
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'm not sure it's a good idea to change this; make format
only works on Linux and Mac, but not on Windows (at least not out of the box).
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.
Thanks for the feed back!
make format only works on Linux and Mac, but not on Windows (at least not out of the box).
I didn't know that.
How about adding a comment like this?
+ # If you are using an environment without make, see the format target in `Makefile`.
go install mvdan.cc/gofumpt@latest && make format
Anyways I want to change this line because I think it's better that no one gets the error shown in the PR body to avoid unnecessary frustration.
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.
To really "fix" it properly, we could add a script to the scripts folder, call it from the Makefile, and add a call to the script here in the readme.
I'm just not sure if it's worth it. We'd rather encourage people to configure their IDE to format on save, that's so much more convenient.
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'm just not sure if it's worth it. We'd rather encourage people to configure their IDE to format on save, that's so much more convenient.
That's true. (and actually I'm configuring so as well.)
I fixed not to use make
target. e779d9b
If you prefer gofumpt -l -w .
, I'll fix so.
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 decided to drop this change completely, I don't want so much noise in the document, and I don't find it important enough.
Also, once we switch to go 1.25 there will be an exclude mechanism that we can use to improve this, which will make your ls-files hack obsolete too; see golang/go#42965 (comment).
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 decided to drop this change completely, I don't want so much noise in the document, and I don't find it important enough.
Thanks for the update for this PR.
I have no objections.
Also, once we switch to go 1.25 there will be an exclude mechanism that we can use to improve this, which will make your ls-files hack obsolete too; see golang/go#42965 (comment).
Thanks for the input. I didn't know that.
e779d9b
to
e9cc07d
Compare
PR Description
The below error message is shown when executing
make format
whentest/_results/demo/worktree_create_from_branches/actual/repo/src/shims.go
exists (maybe executing integration test produces this file?).I have confirmed it works without above error when I run
make format
on this PR's branch.Please check if the PR fulfills these requirements
Cheatsheets are up-to-date (rungo generate ./...
)Code has been formatted (see here)Tests have been added/updated (see here for the integration test guide)Text is internationalised (see here)If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)Docs have been updated if necessary