Maintain expanded attrs/lists/attr params/inherits #224
Conversation
infinisil
force-pushed
the
maintain-expanded-attrs
branch
from
75e1fa1
to
5af9083
Compare
|
Note that the Nixpkgs diff will be misleading for the same reasons as explained in #201 (comment) |
infinisil
force-pushed
the
maintain-expanded-attrs
branch
from
5af9083
to
7081842
Compare
infinisil
changed the title
infinisil
changed the title
infinisil
force-pushed
the
maintain-expanded-attrs
branch
5 times, most recently
from
5082aec
to
d2a63a2
Compare
infinisil
force-pushed
the
maintain-expanded-attrs
branch
2 times, most recently
from
de25dc0
to
2c0d0d1
Compare
infinisil
changed the title
infinisil
force-pushed
the
maintain-expanded-attrs
branch
from
2c0d0d1
to
54d45fc
Compare
There was a problem hiding this comment.
My main point of concern is to make the input preserving features optional for testing purposes. I admit that this is non-trivial to implement though. While we could push these settings through the pretty logic, this would probably require wrapping the entire code in a Reader or something. The alternative would be to encode such conditions in the IR and delay their decision until layoutGreedy. We already do this for optional trailing commas (which depend on the actual layout) for example. Another approach would be to push the option into the parser (which is already wrapped in State) by faking all source lines to 0 if --pure is given.
I haven't looked at the Nixpkgs wide diff but from looking at the test diff already I feel like we may want to introduce a couple of special cases where an expansion shouldn't be preserved: This may happen in situations where expanding made sense in the original format, but looks better collapsed in the RFC format.
| -- Map the last token of that expression, no matter how deep it sits | ||
| -- in the AST. This is useful for modifying comments | ||
| mapLastToken :: (forall b. Ann b -> Ann b) -> a -> a | ||
| mapLastToken f a = fst (mapLastToken' (\x -> (f x, ())) a) |
There was a problem hiding this comment.
Not sure if it should be removed. We've had use for this in the past on and off depending on implementation details of the layout
There was a problem hiding this comment.
We can also just add it back if/when we need it
| pretty (fmap (,hardspace) krec) <> pretty paropen <> sep <> pretty parclose | ||
| where | ||
| -- If the braces are on different lines, keep them like that | ||
| sep = if sourceLine paropen /= sourceLine parclose then hardline else hardspace |
There was a problem hiding this comment.
While I generally see the necessity of preserving parts of the input, it makes reviewing the diff for changes a lot harder. Because now I'll never know if the current change looks the way it does because of the intended output format or because some aspect of the input got preserved.
I'd like to see this feature optional, and have our diff tests run once with and without. This will also ensure that all relevant code paths are more thoroughly tested.
|
@piegamesde In another branch I just experimented with an idea that seems to work pretty well: fb16a35. I don't think this should be blocking for this PR though |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-07-23/49510/1 |
|
What do we do about things like the formatting of pypkgs, which now have a lot of unfortunately contracted attrsets? I think –and not only because of that– my proposed tweaks to the expansion rules should still be implemented. While I don't consider that a blocker, I'd still like to block on at least the local tests additionally running in pure mode. Especially since a lot of future tweaks to the format may get shadowed by this. I'm not 100% fond of your approach to do that since a pure mode will require running the formatter twice (first with shrinkwrap and then again normally) despite a more straightforward way existing, but that can still be worked on later. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/enforcing-nix-formatting-in-nixpkgs/49506/8 |
I agree that some tweaks might be good, but this can be done entirely independently. This PR solves one problem, not all related problems.
I honestly don't have the time to work on finishing such a pure mode. As implemented in fb16a35 it's quite unrefined and has some oddities, such as shifting around of trailing comments, which are just confusing. And other approaches are very tricky to implement. Changing the format really isn't a critical failure that would endanger the stability downstream projects, so I think the tests that I added in this PR are enough. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/formatting-team-meeting-2024-08-06/50222/1 |
piegamesde
force-pushed
the
maintain-expanded-attrs
branch
from
846ae32
to
4fc4f8f
Compare
And remove an unused class method
This is a part of the preparation for a future commit that changes the number of fields of the Ann type
This is a part of the preparation for a future commit that changes the number of fields of the Ann type
By making it a record and not relying on the non-record syntax, we can easily reorder or add new fields to it in the future without breaking any existing patterns. This commit also uses the record update syntax where before an entirely new Ann was constructed, saving a bunch of variables (and implicitly passing through any extra fields added to Ann in the future).
This is finally the new field of Ann that past commits have been working up to. Because it's not an optional field, the ann function is extended to take an extra argument for its initialisation, which then automatically prompts the connection of source locations from the original to the newly created value.
piegamesde
force-pushed
the
maintain-expanded-attrs
branch
from
74f6138
to
0447bde
Compare
| -- Same as mapLastToken, but the mapping function also yields a value that may be | ||
| -- returned. This is useful for getting/extracting values | ||
| mapLastToken' :: (forall b. Ann b -> (Ann b, c)) -> a -> (a, c) | ||
|
|
||
| mapAllTokens :: (forall b. Ann b -> Ann b) -> a -> a | ||
|
|
There was a problem hiding this comment.
It's unfortunate that this needs to be so manual. I've played around with some changes to improve that, very WIP though, just for future reference: 3b61d8a
piegamesde
force-pushed
the
maintain-expanded-attrs
branch
2 times, most recently
from
7fca012
to
9f54daa
Compare
This changes the formatting to take into account whether attribute sets
are expanded onto multiple lines already, and if so, preserves that.
This is desirable because if a user writes an attribute set over
multiple lines, there's most likely an intention to add more attributes,
which would become harder when formatted.
An example of this is module system options, which usually start out
with just a type (or even no attributes), but usually get more
attributes later. This would've been contracted onto a single line
before this change:
options.foo = lib.mkOption {
type = types.str;
};
This change conforms to the standard due to this line:
> The formatter may take the input formatting into account in some cases in order to preserve multi-line syntax elements (which would otherwise have been contracted by the rules).
This changes the formatting to take into account whether lists
are expanded onto multiple lines already, and if so, preserves that.
This is desirable because if a user writes an list over
multiple lines, there's most likely an intention to add more elements,
which would become harder when formatted.
An example of this is build inputs, which usually start out with just a
single element, but usually get more elements later. This would've been
contracted onto a single line before this change:
buildInputs = [
libfoo
];
This change conforms to the standard due to this line:
> The formatter may take the input formatting into account in some cases in order to preserve multi-line syntax elements (which would otherwise have been contracted by the rules).
This changes the formatting to take into account whether attribute
parameters are expanded onto multiple lines already,
and if so, preserves that.
This is desirable because if a user writes an attribute parameter over
multiple lines, there's most likely an intention to add more attributes,
which would become harder when formatted.
An example of this is package recipes, which usually start out with just a
single attribute argument, but usually get more later. This would've been
contracted onto a single line before this change:
{
stdenv,
}:
This change conforms to the standard due to this line:
> The formatter may take the input formatting into account in some cases in order to preserve multi-line syntax elements (which would otherwise have been contracted by the rules).
This changes the formatting to take into account whether inherits
are expanded onto multiple lines already, and if so, preserves that.
This is desirable because if a user writes an inherit over
multiple lines, there's most likely an intention to add more,
which would become harder when formatted.
An example of this is inheriting from `lib`, which usually starts out
with just a single value, but usually gets more later. This would've been
contracted onto a single line before this change:
inherit (lib)
foo
;
This change conforms to the standard due to this line:
> The formatter may take the input formatting into account in some cases in order to preserve multi-line syntax elements (which would otherwise have been contracted by the rules).
This might happen if you temporarily remove all inherits, in which case
the formatter shouldn't remove the spacing that was there.
E.g. I might have this initially
inherit a b;
inherit
c
d
;
removing all the bindings gives me
inherit ; # I already have the space that I'll need
inherit # I can just press enter here to add one
;
which shouldn't be changed, because it's easier like this to insert new
entries again, compared to
inherit;
This also makes it more consistent with the parent commit's change
infinisil
force-pushed
the
maintain-expanded-attrs
branch
from
9f54daa
to
b82144a
Compare
|
Did a minor push to rename a left-over mention of pure mode. @piegamesde Please merge if it looks good to you, I think it's good now :D |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/satisfaction-survey-from-the-new-rfc-166-formatting/49758/16 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/satisfaction-survey-from-the-new-rfc-166-formatting/49758/18 |
|
Could we get this into Nixpkgs' CI please 🙏 ? |
|
@doronbehar NixOS/nixpkgs#336322, but after this, the pinned Nixpkgs needs to be updated once nixpkgs-unstable updates with the new nixfmt version Edit: Done |
This is the alternative suggested in #222 (comment).
This PR changes attribute sets, lists, attribute parameters and inherits that are already on multiple lines to never be contracted to a single line. This is desirable because if a user writes these over multiple lines, there's most likely an intention to add more attributes, which would become harder when formatted.
Examples that would've been contracted to a single line before:
This change conforms to the standard due to
nixfmt/standard.md
Line 67 in f09ce41
This PR is best review commit-by-commit. The first bunch of commits are just code refactorings to make the functional change in later commits as simple as possible.
This work is sponsored by Antithesis ✨