-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Parse inherit names with quotes #107
Conversation
Lucus16
commented
Oct 21, 2022
•
edited by yorickvP
Loading
edited by yorickvP
- CHANGELOG
- simplify names elsewhere
- check if comments are moved correctly
adbb326
to
4652cca
Compare
4652cca
to
9e0eb40
Compare
I just rebased this and it still works, is there any plans on merging this PR? Also I was just wondering, would fixing #97 be a part of this PR or is it a separate piece of work? |
As far as I can tell, |
I added some tests that indicate why |
335d773
to
6edc362
Compare
6edc362
to
ec36303
Compare
I took the freedom to rebase onto master. Code looks good to me, so I'm inclined to merge |
What does "simplify names elsewhere" and "check if comments are moved correctly" mean exactly? And is this a merge blocker? |
This PR simplifies fancy identifiers that are fancy for no reason: In inherit expressions, As for moving comments correctly, when simplifying those names, the tokens that used to be in the fancy identifiers are dropped but they may have had comments attached which must not be dropped. So we need to verify, preferably with a test, that all the possible places comments might have been attached to one of the tokens that are dropped, those comments are still in the output. This is a blocker. Note that each token might have two different kinds of comments attached: leading and trailing. Also, the implementation will probably need to be different after #117. In that case I might put I also just noticed something else. This PR parses and formats identifiers which are also keywords correctly, with quotes. But it does not yet parse or format other quoted strings as identifiers, which Nix does allow. |
Oh okay, I hadn't noticed that it also changes the style of the identifiers. I'd honestly advise against doing such simplifications (and the current RFC 101 draft explicitly declares them out of scope, FWIW), as it has several issues:
I just noticed that Nixfmt does not (properly) handle multiline string identifiers in general (as far as I can tell), I'll go and investigate that |
This PR currently only changes the style of identifiers in inherit expressions, not elsewhere. Regarding superfluous parentheses, I had meant to remove all parentheses during parsing and regenerate them when needed for either correct parsing or for readability. In that case nixfmt would decide for everyone what is readable and what is not, which is pretty doable in the case of operator precedence, since you only need to consider the number of operators squared cases of whether the precedence is obvious. I never got around to implementing it. Of course I understand you won't make the same choice, I'm fine with that. The same goes for quoted identifiers. I tried to build nixfmt to be maximally opinionated so an AST could only have one canonical format, but I understand that does not match the needs of an official nixpkgs formatter, so feel free to make different choices. I do believe that |
To be clear, feel free to change what this PR does. I already consider the Nix formatter team to be the maintainers of nixfmt, including the current version. |
Thank you for your input on this. I'll put it onto the agenda for the next team meeting to discuss before making any changes to the implementation. |
This was discussed in the last meeting. While some AST changes may be okay, this one is simply not worth the effort properly implementing ("bad code does not deserve good formatting"). I re-implemented the change and copied over the tests in #141. Feel free to take a look. |