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

doc/python: mention sorting attribute names #96877

Merged
merged 1 commit into from Mar 1, 2021

Conversation

jonringer
Copy link
Contributor

@jonringer jonringer commented Aug 31, 2020

Motivation for this change

to continue the groundwork of #96850 and make it more official

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace
Copy link
Contributor

But don't we have are sorted quasi-alphabetically to avoid merge conflicts. right above this? I do agree that alphanumeric the better word since there's attributes with numbers also.

@jonringer
Copy link
Contributor Author

I did a sort -f on all of them. I would like to keep it that way :)

@Ma27
Copy link
Member

Ma27 commented Sep 3, 2020

Is there any practical reason behind this? I think that it's just painful to find the right position when adding a new attribute (in all-packages.nix, python-packages.nix or whatever) and any decent editor provides inline-search to jump to the proper position.

@worldofpeace
Copy link
Contributor

We should probably enforce some sort of pre commit hook to make sure it's sorted? Along with maybe a CI check.

@jonringer
Copy link
Contributor Author

Is there any practical reason behind this?

From the perspective of the nix-cli? no
From the perspective of a human? yes.
There's now just one location that "it should be located". Previously I would look at several locations to see which one was "most appropriate", now I can just type the package name until I find no results, and delete a character to find the package which contains the most similar prefix characters

We should probably enforce some sort of pre commit hook to make sure it's sorted?

This may be hard since we would have to "sort on paragraphs, while filtering out comments". It's likely to be do-able, but some of the expressions when I was initially sorting them were hard to handle.

I also thought of including this my nix-template cli tool jonringer/nix-template#5

@Ma27
Copy link
Member

Ma27 commented Sep 3, 2020

There's now just one location that "it should be located". Previously I would look at several locations to see which one was "most appropriate", now I can just type the package name until I find no results, and delete a character to find the package which contains the most similar prefix characters

I asked about the sorting itself. The entire "where do I place my attribute"-thing wouldn't be necessary without it.

@jonringer
Copy link
Contributor Author

I asked about the sorting itself. The entire "where do I place my attribute"-thing wouldn't be necessary without it.

I'm not sure I understand, I sorted all of python-packages.nix in #96850. This PR is just making it "more official" that the expected behavior is have new additions also be sorted.

@FRidh
Copy link
Member

FRidh commented Sep 3, 2020

@Ma27 wonders if there is any value to having it sorted at all.

I think its mostly a matter of preference, at least for me it is. It has happened more than once a package was in the set twice, but under a slightly different name (capitalized or not). This you would spot sooner in case of a sorted list, but its rare it happens.

@jonringer
Copy link
Contributor Author

jonringer commented Sep 3, 2020

I think its mostly a matter of preference, at least for me it is. It has happened more than once a package was in the set twice, but under a slightly different name (capitalized or not). This you would spot sooner in case of a sorted list, but its rare it happens.

I have similar feelings. Making it easier to find packages helps with additions, modifications, and removals.

The "hard work" has already been done, I just want it to be more official.

The entire "where do I place my attribute"-thing wouldn't be necessary without it.

This was added under the "contributing guidelines" and I think it makes more sense if you look at the rendered document. https://github.com/NixOS/nixpkgs/blob/6bc12c461ff497f1c6bafeebca02033bea7b396b/doc/languages-frameworks/python.section.md#contributing-guidelines

@jonringer
Copy link
Contributor Author

I split the first bullet into two, and made the sorting word choice to be stronger.

@SuperSandro2000
Copy link
Member

We should probably enforce some sort of pre commit hook

Some hooks mess with rebasing broken commits so I personally never activated them.

Is there any practical reason behind this?

Less merge conflicts.

@jonringer
Copy link
Contributor Author

jonringer commented Feb 28, 2021

I believe there was a dd-agent and a datadog python package which would have been easier to spot if they were anywhere co-located.

Anyway, the ask now is, "please sort the already sorted package list". Where as before, it was, "please attempt to sort it.... it's not really sorted, but some parts are, and please just find something that is a close-enough match".

I haven't heard any strong objection to this, and it has been open for months now.

@jonringer jonringer merged commit 6ed5503 into NixOS:master Mar 1, 2021
@jonringer jonringer deleted the update-python-docs branch March 1, 2021 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants