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

treewide: revert switch to nixfmt #1494

Closed
wants to merge 1 commit into from

Conversation

GaetanLepage
Copy link
Member

@GaetanLepage GaetanLepage commented May 6, 2024

This PR aims at reverting the changes introduced in #1491.
It is my fault for not having reviewing it with more care.

The reasons for this proposition are the following:

  • The nix fmt command now fails with openTempFileWithDefaultPermissions: permission denied (Read-only file system).
    Although the formatting actually goes through, this prevents someone from knowing whether the formatting "succeeded" or failed. I rely extensively on this in my programming workflow. This is being tracked in Remove recursive mode NixOS/nixfmt#151.
  • This change implies manually rebasing + reformatting every single existing/future PR to account for the now failing git-hook and conflicts. I have a big load of such WIP branches.

Those reasons might feel very personal and sound egoist, but at the same time, I am the one writing the most code in this project and feel like my opinion should matter a little bit.
If some practical solutions to those issues exist (although I haven't find some yet), I would be glad to be proven wrong and close this.

I would then plead for this change to be temporarily reverted.
I want to stress that I am not against switching formatter in the long term. I actually do not care that much for the specific style we use. Although I like a lot the alejandra style, I understand the motivation to eventually mimic the future adoption of nixfmt by nixpkgs.

I also admit that I should have reviewed/tested this more carefully before approving it.

@GaetanLepage GaetanLepage requested a review from traxys May 6, 2024 14:53
@GaetanLepage GaetanLepage deleted the alejandra branch May 7, 2024 20:52
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.

1 participant