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

TEST: reformat with latest nixfmt #2209

Closed
wants to merge 2 commits into from

Conversation

MattSturgeon
Copy link
Member

Just to satisfy my curiosity, this is what the repo looks like when running

nix run github:nixos/nixfmt -- **/*.nix

Note: some of the new formatting uses input-formatting-based heuristics, so if you (for example) wanted to reformat the following, it may require manual intervention.

# from
mkRenamedOptionModule
  [
    "plugins"
    "hello"
    "foo"
  ]
  [
    "plugins"
    "goodbye"
    "bar"
  ]
# to
mkRenamedOptionModule [ "plugins" "hello" "foo" ] [ "plugins" "goodbye" "bar" ]
# or
mkRenamedOptionModule
  [ "plugins" "hello" "foo" ]
  [ "plugins" "goodbye" "bar" ]

We can experiment with additional commits on this branch, manually modifying input formatting and re-running nixfmt to see what it now permits.

@khaneliman
Copy link
Contributor

mkRenamedOptionModule
  [ "plugins" "hello" "foo" ]
  [ "plugins" "goodbye" "bar" ]

This one looks cleanest to me

@MattSturgeon
Copy link
Member Author

Tried playing with some mkRenameOptionModules, I was really hoping NixOS/nixfmt#233 would clean these up (as above). So far I'm kinda disappointed:

(mkRenamedOptionModule [ "foo" "bar" ] [
  "hello"
  "world"
])
(mkRenamedOptionModule [ "foo" "bar" ] (
  foo
  ++ [
    "hello"
    "world"
  ]
))
(mkRenamedOptionModule
  (
    foo
    ++ [
      "bar"
      "baz"
    ]
  )
  [
    "hello"
    "world"
  ]
)

The final argument is always expanded to multiple lines, even if the line length would remain short.

Additionally, any concatenation changes the semantics as far as nixfmt is concerned. Concatenation no longer qualifies as a "list used as a function argument"; instead it is treated as "part of a concatenation expression". It is therefore always expanded to multiple lines 😭

This isn't the best place to discuss nix formatting; we should probably raise this as feedback in the nixfmt repo I guess. Although there's already NixOS/nixfmt#206 and NixOS/nixfmt#228, which are related.

For now I'll just cc some of the formatting team:
@infinisil @piegamesde @emilazy

@emilazy
Copy link

emilazy commented Sep 9, 2024

I'm not a member of the formatting team :) though I play one on TV.

@khaneliman
Copy link
Contributor

I'm not a member of the formatting team :) though I play one on TV.

I was going to try blaming you when I saw you tagged but then saw you weren't the one who did the commit :P

@infinisil
Copy link
Contributor

infinisil commented Nov 29, 2024

With NixOS/nixfmt#257 this is fixed now! Though be aware of NixOS/nixfmt#224

Comment on lines 50 to 51
config_file_path = defaultNullOpts.mkNullable (
with types; either str (listOf str)
) [ ] "Custom config file path or list of custom config file paths.";
config_file_path = defaultNullOpts.mkNullable (with types; either str (listOf str)) [
] "Custom config file path or list of custom config file paths.";
Copy link
Member Author

Choose a reason for hiding this comment

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

TBH, I'm not a huge fan of either of these formats, but breaking within an empty [ ] seems particularly strange. Is that intended @infinisil ?

Copy link
Member Author

@MattSturgeon MattSturgeon Nov 29, 2024

Choose a reason for hiding this comment

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

Personally, I'd like to see long multi-arg function calls like this wrapped with one arg per line, e.g.

config_file_path =
  defaultNullOpts.mkNullable
    (with types; either str (listOf str))
    [ ]
    "Custom config file path or list of custom config file paths.";

or

config_file_path = defaultNullOpts.mkNullable
  (with types; either str (listOf str))
  [ ]
  "Custom config file path or list of custom config file paths.";

Should I raise this in the formatting matrix, open an issue upstream, or has this already been discussed elsewhere?

Choose a reason for hiding this comment

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

Agreed, please open an issue

Copy link
Member Author

@MattSturgeon MattSturgeon Nov 29, 2024

Choose a reason for hiding this comment

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

Sure thing. I've opened NixOS/nixfmt#267 and NixOS/nixfmt#268, for the proposed formatting and weird line-break on empty list, respectively.

This must be done manually, because nixfmt won't "join" lists that are already multi-line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants