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.

Sign up for GitHub

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

Jump to bottom

doc: tweak the coding conventions #51113

Merged
merged 1 commit into from Nov 27, 2018
Merged

doc: tweak the coding conventions #51113

merged 1 commit into from Nov 27, 2018

Conversation

zimbatm
Copy link
Member

@zimbatm zimbatm commented Nov 27, 2018 •
edited

Tweaks the coding style guide to encourage putting container elements on their own lines.

This minimizes diffs, merge conflicts and makes re-ordering of those elements easier.

Nix doesn't suffer the restrictions of other languages where commas are
used to separate list items.

Motivation for this change

Discussion at https://discourse.nixos.org/t/on-nix-expression-formatting/1521/15

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

grahamc
grahamc reviewed Nov 27, 2018
View reviewed changes
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

+1 but would rather not give the impression there is some sort of Tweag style guide influencing this :)

@zimbatm
doc: tweak the coding conventions
80cff57 
Encouraging to put container elements on their own lines to minimize
diffs, merge conflicts and making re-ordering easier.

Nix doesn't suffer the restrictions of other languages where commas are
used to separate list items.
@zimbatm zimbatm changed the title doc: tweag the coding conventions doc: tweak the coding conventions Nov 27, 2018
@zimbatm
Copy link
Member Author

zimbatm commented Nov 27, 2018

oops, typo :)

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/on-nix-expression-formatting/1521/16

grahamc
grahamc approved these changes Nov 27, 2018
View reviewed changes
Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

Love it

alyssais
alyssais reviewed Nov 27, 2018
View reviewed changes
doc/coding-conventions.xml
elem1
elem2
elem3
];
Copy link
Member

@alyssais alyssais Nov 27, 2018 •
edited

Choose a reason for hiding this comment

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

What would you suggest for this common case?

{
  list =
    [
      elem1
      elem2
      elem4
    ] ++ lib.optionals stdenv.isDarwin [ elem4 elem5 ];
}

I like the extra indentation, since it needs top-level keys easy to spot, but if the [ is on the same line as the =, that doesn't really make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

{
  list = [
    elem1
    elem2
    elem3
  ] ++ lib.optionals stdenv.isDarwin [
    elem4
    elem5
  ];
}

looks good to me, though it is kind of similar to

someParameter = [ {
    foo = bar;
    baz = quux;
  } {
    some = more;
    stuff = here;
} ];

Copy link
Member Author

Choose a reason for hiding this comment

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

I think your version is fine.

I only really care about the mechanics of what happens when maintaining the code. So when the code evolves, if elem{1,2,3} are on distinct lines with no [] they are easily movable / commentable / sortable. A soft rule would be that if there are more than 2 elements in a list it's better to use the multi-line notation.

A variation of your example I have seen which is also quite nice is:

{
  list =
    [
      elem1
      elem2
      elem3
    ]
    ++ lib.optionals stdenv.isDarwin [ elem4 elem5 ]
    ++ lib.optionals stdenv.isLinux [ elem6 ]
    ;
}

Again here the same logic applies: each of the extra ++ is easily movable without having to worry of moving the ; around.

@zimbatm zimbatm merged commit 064bd03 into NixOS:master Nov 27, 2018
@zimbatm zimbatm deleted the tweak-coding-convention branch November 27, 2018 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@alyssais alyssais alyssais approved these changes

@ryantm ryantm ryantm approved these changes

@grahamc grahamc grahamc approved these changes

@rycee rycee rycee approved these changes

@c0bw3b c0bw3b c0bw3b approved these changes

Assignees
No one assigned
Labels
8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@zimbatm @nixos-discourse @ryantm @grahamc @jtojnar @rycee @alyssais @c0bw3b @GrahamcOfBorg