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

attribute-ordering: Add attribute ordering check #6

Merged
merged 2 commits into from
Nov 18, 2020
Merged

Conversation

jtojnar
Copy link
Owner

@jtojnar jtojnar commented Nov 17, 2020

We want to use `null` phases in tests to trigger some checks
without changing the builder. Replicating unpackPhase would just be
to annoying.
homepage = "";
license = licenses.mit;
platforms = platforms.unix;
maintainers = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I've noticed that a lot of expressions we've wrote have been left to right in ordering the maintainers, but we really should do the long style no?

Suggested change
maintainers = [];
maintainers = with maintainers; [
somebody
];

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh, yeah. Long style everywhere that changes more than rarely. I probably would not bother with outputs unless there are more than five, or some of them need explanation comment.

msg = ''
The ${lib.optionalString (fstInfo.group != null) "${fstInfo.group}, including the "}attribute “${fst}” should preferably come before ${lib.optionalString (sndInfo.group != null) "${sndInfo.group}’ "}“${snd}” attribute in the expression.

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/attribute-ordering.md
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be interesting if we could turn this into man pages for offline reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope here, though.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Opened #7

@worldofpeace
Copy link
Contributor

worldofpeace commented Nov 18, 2020

The ordering documented here is exactly as what seems to be, i guess logically, the preferred order.

Edit: I thing I remembered that I do is *Inputs and mesonFlags (etc.) are alphabetized. I do this with a editor plugin, but I wonder if it would be a good idea to either mention that in docs. It could be nice to do that with the tool also.

@jtojnar
Copy link
Owner Author

jtojnar commented Nov 18, 2020

I wanted to add meta orderings and output orderings but was not sure if it should all be in attribute-ordering linter. The former maybe, the latter probably not.

*Inputs will be hard, as the Nix code is not aware what are the value names, only the values. I guess we could approximate it by taking drv.name but it would be far from perfect. Ideally, we could also access Nix AST but I have not yet came up with a battle plan (will need to choose between rnix and hnix).

I am still not 100% on mesonFlags alphabetization, there are some benefits to having the same order as meson_options.txt, yet alphabetization makes sense too. I will think on it some more.

@jtojnar jtojnar merged commit 535120b into master Nov 18, 2020
@jtojnar jtojnar deleted the attr-ord branch November 18, 2020 20:39
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.

2 participants