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

Final polish #269

Merged
merged 6 commits into from
Dec 4, 2024
Merged

Final polish #269

merged 6 commits into from
Dec 4, 2024

Conversation

piegamesde
Copy link
Member

Picking some low hanging fixes before the big treewide

Copy link

github-actions bot commented Dec 2, 2024

Nixpkgs diff

Fixes NixOS#266.

One may dislike with the general style, however this undeniably fixes an
inconsistency oversight.
Fixes NixOS#198.

This is a hotfix that should exclude the most common ugly case people
might run into, if this comes up again we should more thoroughly rethink
the rules for this case.
@infinisil
Copy link
Member

before the big treewide

Oh um, but then we need to redo NixOS/nixpkgs#359904, wait for another Hydra rebuild, then redo NixOS/nixpkgs#361165

As much as everyone would wish for, tabs are not whitespace according to
Nix whitespace stripping.
Fixes NixOS#116, but my overall confidence level in Nixfmt's correctness in
parsing the mess which is the Nix syntax remains rather low.
@piegamesde
Copy link
Member Author

Unfortunately, I stumbled upon #116 which I'd now consider a critical correctness issue and therefore blocking. I recently did a Nixpkgs treewide on fucked up strings (for a different variant of indented string syntax fuckup) and therefore know that such things actually still exist in the wild.

Identifiers already were, that the others weren't was a mere oversight.

Fixes NixOS#203
@piegamesde
Copy link
Member Author

As for "Don't move trailing comments in let anymore", my suggestion would be to keep the commit out and thus the old behavior for the initial formatting, and then add it back in. The other commits are strongly recommended to include for the treewide

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the diff, looking good to me, thanks! The third commit some people might not like due to it introducing more lines, but I think it's the right move to fix the reported problem (#198).

src/Nixfmt/Pretty.hs Show resolved Hide resolved
@infinisil infinisil merged commit a463903 into NixOS:master Dec 4, 2024
4 checks passed
@infinisil infinisil deleted the final-polish branch December 4, 2024 21:04
infinisil added a commit to tweag/nixpkgs that referenced this pull request Dec 4, 2024
nix-backports bot pushed a commit to NixOS/nixpkgs that referenced this pull request Dec 4, 2024
getchoo pushed a commit to NixOS/nixpkgs that referenced this pull request Dec 5, 2024
…361894)

nixfmt-rfc-style: 2024-11-26 -> 2024-12-04

Final polish: NixOS/nixfmt#269

(cherry picked from commit cb3de35)

Co-authored-by: Silvan Mosberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants