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

Always expand attrs #222

Closed
wants to merge 2 commits into from
Closed

Always expand attrs #222

wants to merge 2 commits into from

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jul 17, 2024

Note

Closed in favor of #224

This implements this part of the standard:

nixfmt/standard.md

Lines 1259 to 1267 in f09ce41

- Attribute set values must always be expanded. This has the consequence of always forcing nested attribute sets to be multiline (even if they would be single line otherwise because they only contain a single item), which usually is desired.
```nix
{
foo.bar.baz = "qux";
foo' = {
bar.baz = "qux";
};
}
```

This inconsistency was noticed in NixOS/nixpkgs#326430 (comment)

Edit: Though there's also this here:

- Lists and attribute sets can only be on a single line if they fit on the line and contain few enough items.

Copy link

github-actions bot commented Jul 17, 2024

Nixpkgs diff

@infinisil infinisil force-pushed the always-expand-attrs branch from cc0507a to 2f67380 Compare July 17, 2024 02:31
@infinisil
Copy link
Member Author

As a better alternative to this, I think we should finally implement input-aware formatting of lists/sets. Specifically to never contract lists/sets that are expanded onto multiple lines already.

We previously added this to the standard to make that possible:

- 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).

Copy link
Member

@0x4A6F 0x4A6F left a comment

Choose a reason for hiding this comment

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

Thanks

@piegamesde
Copy link
Member

The part of the standard you quote is taken out of context, this is a deliberate special case for bindings. IMO this sweeping approach of always expanding attributes is a net negative to the output layout, especially when attribute sets are non-last arguments to function calls.

Moreover, this misses several other similar inconsistencies which happen with lists.

The current rules have already been tweaked a lot, and I would strongly prefer an approach which refines them a bit more:

  • Use prettyTermWide in prettyApp in contexts where prettyTermWide is already used (e.g. bindings)
  • Expand the logic to also apply let binding bodies, in operator chains(?), and for lists

@infinisil
Copy link
Member Author

Implemented the above alternative here: #224

@infinisil
Copy link
Member Author

Closing in favor of the above alternative

@infinisil infinisil closed this Jul 19, 2024
@infinisil infinisil deleted the always-expand-attrs branch July 19, 2024 04:04
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