-
Notifications
You must be signed in to change notification settings - Fork 616
Run gofumpt via go tool in dprint #586
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
b38d42a
to
32a1493
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.
Pull Request Overview
This PR updates how gofumpt is executed in dprint by leveraging the faster "go tool" invocation, while also updating related dependency versions.
- Upgraded several dependencies in go.mod, including golang.org/x/sys, golang.org/x/mod, golang.org/x/sync, golang.org/x/tools, and added mvdan.cc/gofumpt v0.8.0.
- Removed the explicit mvdan.cc/gofumpt version specification from Herebyfile.mjs to rely on the go.mod dependency.
- Updated the .dprint.jsonc configuration to run gofumpt via "go tool" and removed an obsolete tool installation step from the CI workflow.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
File | Description |
---|---|
go.mod | Upgraded dependencies and added mvdan.cc/gofumpt to ensure consistent tool versions across use |
Herebyfile.mjs | Removed the explicit mvdan.cc/gofumpt version to avoid duplication and rely on go.mod versioning |
.github/workflows/ci.yml | Removed the "npx hereby install-tools" step to align with dependency management changes |
.dprint.jsonc | Updated the command to run gofumpt via "go tool" for improved speed and caching |
Comments suppressed due to low confidence (1)
Herebyfile.mjs:130
- The removal of the explicit version specification for mvdan.cc/gofumpt in this file aligns with the update in go.mod to v0.8.0. Confirm that all references to the tool use the go.mod dependency to prevent version mismatches.
["mvdan.cc/gofumpt", "v0.7.0"]
I think I want to merge this to make things easier. For formatting the whole codebase:
But, for our largest file:
Given incremental mode won't format files that haven't changed, most of the time dprint will be quick. So I think the perf is actually totally fine. Here's what happens if I have it touch
|
How do you do this? |
This is just in:
|
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 run hereby format
2-3x per PR, so some slowdown is worth it to me in order to never have to think about whether something is installed or not. (I also have "formatting.gofumpt": true
in workspace settings.)
I'm confused, those two are contradictory; this PR makes it a little slower (but only when the formatter is called many times) |
This is a lot slower;
go tool
is faster thango run
from 1.23, but still a lot slower than a plain binary. But, this doesn't require any global installs ofgofumpt
to rundprint
. Given the recommendation is to configuregopls
to rungofumpt
instead, anddprint
does cache things, it seems like this isn't such a bad tradeoff to avoiddprint fmt
failing unexpectedly for those using that CLI.EDIT: see #586 (comment) for perf info; most of the time it's fine.