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

CLI option to format non-singleton lists on one line #206

Open
MattSturgeon opened this issue Jun 4, 2024 · 6 comments
Open

CLI option to format non-singleton lists on one line #206

MattSturgeon opened this issue Jun 4, 2024 · 6 comments
Labels
style Style discussion

Comments

@MattSturgeon
Copy link

MattSturgeon commented Jun 4, 2024

Description

In the same spirit as --width (default 100) allowing end-users outside of nixpkgs to tweak nixfmt's behavior, I'd like an option to disable the default formatting of non-singleton lists.

When the option is enabled, lists of any element count should be rendered on a single line unless that cannot be done for some other reason, such as comments within the list or the total line length being too long.

Better yet, a granular --max-list-items option could be added:

  • default 1 for the current behavior
  • 0 would always produce a multi-line list
  • a negative value (-1) would always produce a single-line list (if possible, as described in the rest of this issue).
  • Any other positive integer would use that value as the threshold for when to no longer attempt to format a list on a single line.

Small example input

{
  single = ["a"];
  short = ["a" "b" "c"];
  long = ["a very very" "long list of" "a few elements" "with many many" "characters!"];
  shortWithComment = [
    # Elements
    "a" "b" "c"
  ];
  shortListWithAVeryVeryLongName_LoremipsumdolorsitametconsecteturadipiscingelitPhasellusaliquetutmauriseumollisSuspendisserhoncusdui = ["a" "b" "c"];
}

Expected output

{
  single = [ "a" ];
  short = [ "a" "b" "c" ];
  long = [
    "a very very"
    "long list of"
    "a few elements"
    "with many many"
    "characters!"
  ];
  shortWithComment = [
    # Elements
    "a"
    "b"
    "c"
  ];
  shortListWithAVeryVeryLongName_LoremipsumdolorsitametconsecteturadipiscingelitPhasellusaliquetutmauriseumollisSuspendisserhoncusdui =
    [ "a" "b" "c" ];
}

Actual output

{
  single = [ "a" ];
  short = [
    "a"
    "b"
    "c"
  ];
  long = [
    "a very very"
    "long list of"
    "a few elements"
    "with many many"
    "characters!"
  ];
  shortWithComment = [
    # Elements
    "a"
    "b"
    "c"
  ];
  shortListWithAVeryVeryLongName_LoremipsumdolorsitametconsecteturadipiscingelitPhasellusaliquetutmauriseumollisSuspendisserhoncusdui = [
    "a"
    "b"
    "c"
  ];

Note: Only the short list and the short list with a long name should be formatted differently when --single-line-lists is used.

Related

This is essentially the opposite of #136 which was asking to revert d2e8575

@MattSturgeon MattSturgeon changed the title --single-line-lists option to format non-singleton lists on one line where possible CLI option to format non-singleton lists on one line where possible Jun 4, 2024
@MattSturgeon MattSturgeon changed the title CLI option to format non-singleton lists on one line where possible CLI option to format non-singleton lists on one line Jun 4, 2024
@rhendric
Copy link
Member

rhendric commented Jul 25, 2024

This should just be the standard behavior, not something to enable on the command line.

standard.md:

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

‘Few enough’ is vague, but ‘at most one’ seems like an inaccurate interpretation.

@infinisil
Copy link
Member

Fwiw, in the latest version, the output is this:

{
  single = [ "a" ];
  short = [
    "a"
    "b"
    "c"
  ];
  long = [
    "a very very"
    "long list of"
    "a few elements"
    "with many many"
    "characters!"
  ];
  shortWithComment = [
    # Elements
    "a"
    "b"
    "c"
  ];
  shortListWithAVeryVeryLongName_LoremipsumdolorsitametconsecteturadipiscingelitPhasellusaliquetutmauriseumollisSuspendisserhoncusdui = [
    "a"
    "b"
    "c"
  ];
}

That should satisfy the standard

@rhendric
Copy link
Member

IMO it would be better if the formatter did not turn

short = [ "a" "b" "c" ];

into

short = [
  "a"
  "b"
  "c"
];

as this is a list with ‘few enough’ items that fits on the line.

Per #224, I don't necessarily expect nixfmt to collapse such lists if they start out expanded; I just think it ought not to expand them if they start collapsed.

@MattSturgeon
Copy link
Author

MattSturgeon commented Jul 25, 2024

Fwiw, in the latest version, the output is this:

Oops, the only difference to my "actual output" was a copy-paste mistake on my end; I had my "expected" and "actual" cases mixed up for the last attr (short list long name).

I've edited the issue description to correct it.

That should satisfy the standard

I believe #206 (comment) is saying that the standard permits having more than one list item on a single line, e.g:

{
  short = [ "a" "b" "c" ];
}

Personally, I think this should be configurable via an option such as --max-list-items.
We can debate what the default should be for such an option though, so long as the default complies with the rfc.

I'd also be happy to have the behavior changed without adding an option, if it proved disproportionately difficult to implement.

@nixos-discourse
Copy link

This issue 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/15

@iFreilicht
Copy link

iFreilicht commented Oct 11, 2024

I would like to add a concrete example why this feature would be helpful, but I would argue it doesn't require a new flag. It should be the default unless --strict is passed.

In nixpkgs, we communicate renamed options like this:

  imports = [
    (mkRenamedOptionModule [ "boot" "loader" "gummiboot" "enable" ] [ "boot" "loader" "systemd-boot" "enable" ])
    (lib.mkChangedOptionModule
      [ "boot" "loader" "systemd-boot" "memtest86" "entryFilename" ]
      [ "boot" "loader" "systemd-boot" "memtest86" "sortKey" ]
      (config: lib.strings.removeSuffix ".conf" config.boot.loader.systemd-boot.memtest86.entryFilename)
    )
    (lib.mkChangedOptionModule
      [ "boot" "loader" "systemd-boot" "netbootxyz" "entryFilename" ]
      [ "boot" "loader" "systemd-boot" "netbootxyz" "sortKey" ]
      (config: lib.strings.removeSuffix ".conf" config.boot.loader.systemd-boot.netbootxyz.entryFilename)
    )
  ];

This is easy to read and feels appropriate to the complexity of the code. Some modules have tens of these lines. If I apply the above example with the current nixfmt (version nixpkgs-unstable-2024-08-16) rules, it expands to a comical 50 lines of code:

    imports = [
    (mkRenamedOptionModule
      [
        "boot"
        "loader"
        "gummiboot"
        "enable"
      ]
      [
        "boot"
        "loader"
        "systemd-boot"
        "enable"
      ]
    )
    (lib.mkChangedOptionModule
      [
        "boot"
        "loader"
        "systemd-boot"
        "memtest86"
        "entryFilename"
      ]
      [
        "boot"
        "loader"
        "systemd-boot"
        "memtest86"
        "sortKey"
      ]
      (config: lib.strings.removeSuffix ".conf" config.boot.loader.systemd-boot.memtest86.entryFilename)
    )
    (lib.mkChangedOptionModule
      [
        "boot"
        "loader"
        "systemd-boot"
        "netbootxyz"
        "entryFilename"
      ]
      [
        "boot"
        "loader"
        "systemd-boot"
        "netbootxyz"
        "sortKey"
      ]
      (config: lib.strings.removeSuffix ".conf" config.boot.loader.systemd-boot.netbootxyz.entryFilename)
    )
  ];

I would expect nixfmt to leave the short lists collapsed as long as the resulting line is shorter than --width. If --strict is passed, the lists should still be expanded. I don't really mind if it expands the function, though that also seems unnecessary in many cases:

  imports = [
    (lib.mkRenamedOptionModule [ "security" "pam" "enableU2F" ] [ "security" "pam" "u2f" "enable" ])
    (lib.mkRenamedOptionModule [ "security" "pam" "enableSSHAgentAuth" ] [ "security" "pam" "sshAgentAuth" "enable" ])
    (lib.mkRenamedOptionModule [ "security" "pam" "u2f" "authFile" ] [ "security" "pam" "u2f" "settings" "authfile" ])
    (lib.mkRenamedOptionModule [ "security" "pam" "u2f" "appId" ] [ "security" "pam" "u2f" "settings" "appid" ])

All of these are under 100 chars wide, having them collapsed like this aids readability IMO.

@piegamesde piegamesde added the style Style discussion label Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style Style discussion
Projects
Status: Todo
Development

No branches or pull requests

6 participants