Skip to content
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

A --check flag that won't actually apply the formatting but only tell us if some files need formatting and if possible, the list of those #500

Open
jraygauthier opened this issue Dec 19, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@jraygauthier
Copy link

jraygauthier commented Dec 19, 2024

Same behavior as any formatter's --check flag. If the formatter does not provide a --check flag, the formatting could be done to a temp location and a diff helper could be used to check if there are differences.

Same reason why --check mode is needed for formatter. I don't want my CI to mutate the files (which is the current behavior of the --ci flag).

@jraygauthier jraygauthier added the enhancement New feature or request label Dec 19, 2024
@brianmcgee
Copy link
Member

There isn't really a uniform approach for --check across formatters, so the current --ci approach is just to run the formatters and see if anything has changed.

I'd like to understand better why mutating the files in CI is an issue for you.

@jraygauthier
Copy link
Author

There isn't really a uniform approach for --check across formatters

I wasn't aware of this fact. All formatters I used to date always provided some option (usually --check) to dry run the formatting and report if something would change were I to apply the actual format operation. The bare minimum (common point of all) was to return a non 0 exit status to indicate changes are required.

I'd like to understand better why mutating the files in CI is an issue for you.

Our approach is that the CI reuse the same logic as the local developer through a justfile run within a nix develop reproducible environment. Basically, both the developer and CI run the exact same code to validate the build, which is the default just task: $ just. The default just task usually perform some non versioned file initialization (as/if needed), the build (when applicable), run some static checks and then run the tests. We run our check-format task (where the --check behavior would be required) alongside other static checkers such as linter and type checkers. None of the sub tasks run by the default just task touch any the version controlled code. We provide a separate format task (this is where the current treefmt behavior is needed) if the developer needs to format the code base (in addition to optional IDE integrations). We believe that any change to source code must come from an explicit developer action and want to avoid unintended side effect. Our CI never mutate the source code and we intend to keep it that way. It will only tell if the current changeset meet the minimal standards (as defined by the default justfile task).

In the meantime, we need to manually call individual formatters in our check-format task which kind of defeat the purpose of treefmt (avoid repeated boilerplate). We were a bit disappointed when we understood treefmt did not expose any way to use the underlying formatter' --check mode when all of the required formatter provide one. Note that even if the formatter would provide this option, it would still be possible for treefmt to provide a nice fallback that make use of a diff based (or go specific diff tool) approach.

@brianmcgee
Copy link
Member

I'll dig into this a bit more over the holidays. @zimbatm @jfly I'd like your take as well.

A simple pass-through is possible if -check is well supported. Where it gets trickier, is having to support formatters which don't.

On a related note though, this sounds like what treefmt-nix already has some plumbing for. It adds a flake check which fails and produces a diff if there were changes (within a nix build, so it doesn't touch the local filesystem).

Could that be of use in this instance @jraygauthier ?

@brianmcgee brianmcgee self-assigned this Dec 20, 2024
@jraygauthier
Copy link
Author

jraygauthier commented Dec 20, 2024

Could that be of use in this instance @jraygauthier ?

Thanks for the ref, I wasn't aware it was available. Will have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants