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

treefmt cache results in warnings getting suppressed #433

Closed
jfly opened this issue Oct 10, 2024 · 7 comments · Fixed by #447
Closed

treefmt cache results in warnings getting suppressed #433

jfly opened this issue Oct 10, 2024 · 7 comments · Fixed by #447
Labels
bug Something isn't working

Comments

@jfly
Copy link
Collaborator

jfly commented Oct 10, 2024

Describe the bug

treefmt helpfully logs warnings when it doesn't know how to format a file:

$ treefmt test.py
WARN format: no formatter for path: test.py
traversed 1 files
emitted 1 files for processing
formatted 0 files (0 changed) in 21ms

However, if you try to format that exact same file again, you won't see the warning:

$ treefmt test.py
traversed 1 files
emitted 0 files for processing
formatted 0 files (0 changed) in 13ms

Note how the warning reappears if you invoke treefmt with --no-cache:

$ treefmt --no-cache test.py
WARN format: no formatter for path: test.py
traversed 1 files
emitted 1 files for processing
formatted 0 files (0 changed) in 3ms

To Reproduce

Run treefmt twice on the same file.

Expected behavior

I'd like to see the same set of warnings every time, regardless of the state of my treefmt cache.

System information

I'm using the the latest code on main (714cf33 at time of writing).

@jfly jfly added the bug Something isn't working label Oct 10, 2024
@brianmcgee
Copy link
Member

I need to fish out the issue, but I remember this being discussed and someone arguing for the "warn once, but don't annoy me every time" version, which it currently does. It's a toss-up, really. I can see the value of both approaches.

I don't want to be ping-ponging between enabling and disabling this every so often.

@brianmcgee
Copy link
Member

brianmcgee commented Oct 12, 2024

That being said, with #431, it is now easier to provide persistent config either in the treefmt.toml file or via the environment, so you can choose which behaviour you want and not have to give any extra flags each time.

Perhaps a cache-unmatched option is false by default, but can you silence it quickly enough? 🤔

@jfly
Copy link
Collaborator Author

jfly commented Oct 12, 2024

Speaking as a user of a piece of software, I am surprised if the existence of a cache affects anything other than the performance of the software. This is a situation where the state of the cache affects the actual output of the software, which smells odd to me.

I remember this being discussed and someone arguing for the "warn once, but don't annoy me every time" version, which it currently does

If people want a "warn me once for each new file" behavior, that feels like it should be implemented with state, not a cache. The current implementation doesn't even achieve the "warn me once" behavior, because it'll re-warn me about files every time I change those files (because it busts treefmt's cache).

But ignoring warnings, maybe we can agree that --on-unmatched=fatal should behave consistently regardless of cache? Right now, it doesn't.

With a warm cache, treefmt runs successfully:

./ $ treefmt --on-unmatched=fatal
traversed 3 files
emitted 0 files for processing
formatted 0 files (0 changed) in 14ms

./ $ echo $status
0

With no cache, it errors out about my (cached) .envrc file which it doesn't know how to format:

./ $ treefmt --on-unmatched=fatal --no-cache
traversed 3 files
emitted 3 files for processing
formatted 0 files (0 changed) in 1ms
Error: no formatter for path: .envrc

./ $ echo $status
1

@jfly
Copy link
Collaborator Author

jfly commented Oct 12, 2024

That being said, with #431, it is now easier to provide persistent config either in the treefmt.toml file or via the environment, so you can choose which behaviour you want and not have to give any extra flags each time.

I do agree that helps! My suggestion would be for treefmt to always warn about these things, and folks can turn the warnings off if they want.

@brianmcgee
Copy link
Member

I agree about the bahviour being inconsistent.

With the recent changes it should be straightforward to always log, and the user can silence the warning by changing the level in their config or an env variable.

@brianmcgee
Copy link
Member

There will be a performance hit, but how much, I'm not sure. This would require us to always apply the matching rules to every file rather than only matching on files that have changed. I'll play around with it later today and try it out on nixpkgs.

@brianmcgee
Copy link
Member

brianmcgee commented Oct 13, 2024

I made this change in #447.

The performance impact of matching before checking the cache appears to be negligible when run against Nixpkgs:

# beforenix run github:numtide/treefmt -- --config-file ./test/examples/nixpkgs.toml --tree-root /home/brian/Development/com/github/nixos/nixpkgs -u info -c 
traversed 43492 files
emitted 43492 files for processing
formatted 36059 files (21522 changed) in 21.107snix run github:numtide/treefmt -- --config-file ./test/examples/nixpkgs.toml --tree-root /home/brian/Development/com/github/nixos/nixpkgs -u info  
traversed 43492 files
emitted 0 files for processing
formatted 0 files (0 changed) in 298ms

# afternix run .# -- --config-file ./test/examples/nixpkgs.toml --tree-root /home/brian/Development/com/github/nixos/nixpkgs -u info -c 
traversed 43492 files
emitted 43492 files for processing
formatted 36059 files (21522 changed) in 21.031s

# after with hot cachenix run .# -- --config-file ./test/examples/nixpkgs.toml --tree-root /home/brian/Development/com/github/nixos/nixpkgs -u info   
traversed 43492 files
emitted 0 files for processing
formatted 0 files (0 changed) in 282ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants