Skip to content

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

Merged
merged 5 commits into from
May 23, 2025
Merged

Run gofumpt via go tool in dprint #586

merged 5 commits into from
May 23, 2025

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Mar 13, 2025

This is a lot slower; go tool is faster than go run from 1.23, but still a lot slower than a plain binary. But, this doesn't require any global installs of gofumpt to run dprint. Given the recommendation is to configure gopls to run gofumpt instead, and dprint does cache things, it seems like this isn't such a bad tradeoff to avoid dprint fmt failing unexpectedly for those using that CLI.

EDIT: see #586 (comment) for perf info; most of the time it's fine.

@jakebailey jakebailey force-pushed the jabaile/gofumpt-go-tool branch from b38d42a to 32a1493 Compare April 18, 2025 17:43
@jakebailey jakebailey marked this pull request as ready for review May 16, 2025 19:41
@Copilot Copilot AI review requested due to automatic review settings May 16, 2025 19:41
Copy link
Contributor

@Copilot Copilot AI left a 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"]

@jakebailey
Copy link
Member Author

I think I want to merge this to make things easier.

For formatting the whole codebase:

Benchmark 1: dprint fmt --incremental=false
  Time (mean ± σ):     261.0 ms ±  17.4 ms    [User: 141.6 ms, System: 91.4 ms]
  Range (min … max):   241.3 ms … 294.9 ms    10 runs
 
Benchmark 2: dprint fmt --incremental=false
  Time (mean ± σ):      4.473 s ±  0.114 s    [User: 0.179 s, System: 0.069 s]
  Range (min … max):    4.302 s …  4.669 s    10 runs
 
Summary
  dprint fmt --incremental=false ran
   17.14 ± 1.22 times faster than dprint fmt --incremental=false

But, for our largest file:

Benchmark 1: dprint fmt --incremental=false ./internal/checker/checker.go
  Time (mean ± σ):     146.6 ms ±   2.7 ms    [User: 15.1 ms, System: 11.6 ms]
  Range (min … max):   144.1 ms … 153.3 ms    10 runs
 
Benchmark 2: dprint fmt --incremental=false ./internal/checker/checker.go
  Time (mean ± σ):     295.8 ms ±  34.2 ms    [User: 14.0 ms, System: 11.9 ms]
  Range (min … max):   232.7 ms … 338.9 ms    10 runs
 
Summary
  dprint fmt --incremental=false ./internal/checker/checker.go ran
    2.02 ± 0.24 times faster than dprint fmt --incremental=false ./internal/checker/checker.go

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 checker.go each time but with incremental enabled:

Benchmark 1: dprint fmt
  Time (mean ± σ):     295.2 ms ±   8.1 ms    [User: 21.2 ms, System: 62.0 ms]
  Range (min … max):   281.0 ms … 306.8 ms    10 runs
 
Benchmark 2: dprint fmt
  Time (mean ± σ):     550.1 ms ±  56.3 ms    [User: 15.0 ms, System: 76.1 ms]
  Range (min … max):   477.0 ms … 635.7 ms    10 runs
 
Summary
  dprint fmt ran
    1.86 ± 0.20 times faster than dprint fmt

@andrewbranch
Copy link
Member

Given the recommendation is to configure gopls to run gofumpt instead

How do you do this?

@jakebailey
Copy link
Member Author

Given the recommendation is to configure gopls to run gofumpt instead

How do you do this?

This is just in:

"formatting.gofumpt": true,

Copy link
Member

@sandersn sandersn left a 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.)

@jakebailey
Copy link
Member Author

I'm confused, those two are contradictory; this PR makes it a little slower (but only when the formatter is called many times)

@jakebailey jakebailey added this pull request to the merge queue May 23, 2025
Merged via the queue into main with commit 52db494 May 23, 2025
23 checks passed
@jakebailey jakebailey deleted the jabaile/gofumpt-go-tool branch May 23, 2025 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants