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

Add a watch mode for automatic formatting on file save #509

Open
shikanime opened this issue Jan 17, 2025 · 9 comments · May be fixed by #510
Open

Add a watch mode for automatic formatting on file save #509

shikanime opened this issue Jan 17, 2025 · 9 comments · May be fixed by #510
Labels
enhancement New feature or request

Comments

@shikanime
Copy link

shikanime commented Jan 17, 2025

Is your feature request related to a problem? Please describe.

Yes, currently treefmt requires manual execution to format the code.

Describe the solution you'd like

I propose to add a new format watch mode to treefmt. This mode would allow treefmt to automatically format the code whenever the file is saved to disk. This could be achieved by integrating with IDEs or using a file watcher to detect changes, perhaps using watchman.

Describe alternatives you've considered

One alternative is to manually format the code after each save. However, this is inefficient and easy to forget. Another alternative is to use an IDE extension that provides automatic formatting for treefmt. However, treefmt-nix doesn't export the config file, so implementing it in every IDE would be double work.

Additional context

Afaik, it would be implementing a new walk provider that returns an unbounded number of files to process until we kill the process. If it's relevant, I have a good enough idea of how to implement it if you'd like.

@shikanime shikanime added the enhancement New feature or request label Jan 17, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 18, 2025
@shikanime shikanime linked a pull request Jan 18, 2025 that will close this issue
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 18, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 18, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 18, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 18, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 18, 2025
@jfly
Copy link
Collaborator

jfly commented Jan 18, 2025

I'm (lightly) opposed to this feature. IMO, it's something your text editor/IDE should be doing instead.

However, treefmt-nix doesn't export the config file, so implementing it in every IDE would be double work.

Which editor do you use? Neovim now has support for this (via none-ls): https://github.com/nvimtools/none-ls.nvim/blob/main/doc/BUILTINS.md#nix_flake_fmt, and it doesn't create double work for you. Perhaps you could implement something similar for your editor?

@shikanime
Copy link
Author

shikanime commented Jan 19, 2025

I don't think I'm representative of most of the community. I regularly switch between vscode, neovim and sometimes helix. And I use nix fmt a lot, but have found the nix flake command to be particularly slow to start compared to running treefmt itself. Having a watch method gives more room for optimisation, as not re-scanning the repository and other startup routines.

@jfly
Copy link
Collaborator

jfly commented Jan 20, 2025

And I use nix fmt a lot, but have found the nix flake command to be particularly slow to start compared to running treefmt itself.

I encourage you to try the nix_flake_fmt builtin for none-ls for neovim that I mentioned earlier. It immediately discovers the nix fmt entry point on startup, and caches the result. In practice, I never find that I have to wait at all for it, because I usually take a few moments after opening a file before I make a change.

As for switching between editors, if they don't already have support for nix fmt, I'd encourage you to look into building something like that. The nix community would benefit from it, and it wouldn't even be a treefmt specific thing you'd be building.

@brianmcgee
Copy link
Member

And I use nix fmt a lot, but have found the nix flake command to be particularly slow to start compared to running treefmt

I was speaking with another user who was finding running via nix fmt was a lot slower, and they showed me an example where it was a good few hundred milliseconds slower.

I think this feature would be good to have, happy to review when you feel #510 is ready.

shikanime added a commit to shikanime/treefmt that referenced this issue Jan 21, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 21, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 22, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 22, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 22, 2025
@shikanime
Copy link
Author

I have just tried out my idea for an implementation, but it seems that I need to refactor the whole processing workflow. Currently, the implementation waits for the walk to finish before starting the formatting and we need to run in stream way.

shikanime added a commit to shikanime/treefmt that referenced this issue Jan 22, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 22, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 22, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 23, 2025
@brianmcgee
Copy link
Member

It should batch things up and kick them off in parallel.

I'll try and find some time to have a look at your branch.

shikanime added a commit to shikanime/treefmt that referenced this issue Jan 23, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 23, 2025
@brianmcgee
Copy link
Member

brianmcgee commented Jan 23, 2025

@shikanime I looked at your branch and played around a bit locally.

I can't push my work-in-progress changes. I think the PR has an option to allow maintainers to push.

By default, treefmt waits for a batch to fill up before formatting. If the number of files to process is less than the batch size (default 1024), the call to close the formatter will flush the final batch.

The processing pipeline doesn't need to be changed. Instead, we must reduce the batch size to 1 when operating in watch mode. This will ensure any batches being produced by the walker are formatted immediately.

With that in place, the new problem is an infinite formatting loop where the watch keeps detecting the file being formatted, thus forcing another format.

We currently track changes with a simple heuristic based on modtime. This works well and has low overhead for how treefmt has been used up until now.

To avoid this infinite loop, we will need to make the change detection configurable and, when in watch mode, switch to a content-based detection method so we only format when there has been a material change to the underlying path.

Introducing the configurable change detection strategy in a separate PR might be best. There is also a stale PR for making batch sizes configurable in general. Perhaps both should be implemented first 🤔

shikanime added a commit to shikanime/treefmt that referenced this issue Jan 23, 2025
shikanime added a commit to shikanime/treefmt that referenced this issue Jan 23, 2025
@shikanime
Copy link
Author

shikanime commented Jan 23, 2025

Setting the batch size to 1 will work, but won't the process stop on the first iteration? As you can see, I tried something in the PR of #511, it works, but it's dirty and seems to break the expectations of the unit tests.

Oh yeah, you're right, I completely forgot that formatting is also a write change, so it gets picked up by fsnotify at least once, I saw that during my test in the last PR. Assuming most of the formatters doesn't reformat files that don't need to be formatted, it's fine? Or is this assumption completely wrong in most or some cases?

Good question about batch size, maybe it could be? Should the user be able to configure it from a ux perspective?

@brianmcgee
Copy link
Member

Reading from walkers is supposed to keep happening until they return an io.EOF.

In #512 I cleaned up the logic around that section.

In my local tweaks of #510, I had to make a minor adjustment, but I can't remember exactly what (I'm on a different device). All tests are passing. I can share it tomorrow on a separate branch or a gist.

The formatters aren't responsible for determining whether a file needs to be reformated. That is something treefmt provides to ensure uniformity of behaviour regardless of implementation.

We have a caching layer that wraps the walkers to filter out unnecessary formatting. As mentioned above, this must be adjusted to operate content-based when in watch mode to prevent constant re-formatting.

Currently, it leans on the modtime for a quick comparison, as treefmt is typically used as a one-shot against a whole repo. I want to implement the configurable comparison strategy in the caching layer first, as it's been requested before, has value in itself, and makes watch mode easier to finish.

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

Successfully merging a pull request may close this issue.

3 participants