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

Sort the maintainer-list #77784

Closed
wants to merge 1 commit into from
Closed

Conversation

yorickvP
Copy link
Contributor

The maintainer-list has a comment about keeping it sorted. Sadly, it still got unsorted.
I have sorted it using nix eval '(import <nixpkgs/maintainers/maintainer-list.nix>)' | nixfmt, so that we can all sleep soundly again.

cc: @grahamc

TODO: investigate impact on maintainer scripts running git blame on this list.

@yorickvP
Copy link
Contributor Author

Update: this splits all the capitalized people from their lowercase friends. I'll investigate having is sort in a case-insensitive way.

@yorickvP
Copy link
Contributor Author

Update: I have resorted (to) using sed and sort. My hope is that order is now restored to this list.

@infinisil
Copy link
Member

I'm not sure if it's worth doing this. I think either we have a check that prevents people from evaluating it when it's unsorted, or we don't keep it sorted. I personally don't see the reason behind keeping it sorted, so I'd prefer the latter.

@yorickvP
Copy link
Contributor Author

yorickvP commented Jan 15, 2020

git blame maintainer-list.nix -ignore-rev HEAD -> 63 lines attributed to me. Some are justified (literally?), others are just moved too far.

@Ma27
Copy link
Member

Ma27 commented Jan 16, 2020

Wouldn't this cause merge conflicts for each PR which adds a new maintainer?

Apart from that, I agree with @infinisil: any decent editor should've some kind of search feature, finding the appropriate location for a new attribute takes IMHO way more time.

@mkaito
Copy link
Contributor

mkaito commented Jan 16, 2020

I think it should have been sorted in the first place, with a CI check. But that's just personal opinion. I don't like it when things are... disorderly. But this is all just bikeshedding anyway.

@alyssais
Copy link
Member

I think it should have been sorted in the first place, with a CI check. But that's just personal opinion. I don't like it when things are... disorderly. But this is all just bikeshedding anyway.

This is unlikely to work with a file changed this often, for the same reason all-packages.nix can’t be sorted. You can test that it’s sorted at the HEAD of the PR, but unless you want to rerun CI against the merge for every PR that touches this file every time any commit is added to master that touches it, it will still get unsorted when merged.

@7c6f434c
Copy link
Member

@alyssais not sure the situation you describe can happen without causing a merge conflict.

@alyssais
Copy link
Member

alyssais commented Jan 16, 2020 via email

@7c6f434c
Copy link
Member

7c6f434c commented Jan 17, 2020 via email

@timokau
Copy link
Member

timokau commented Jan 23, 2020

I'd also say its not worth doing it, at least not in a one-off fashion. If we would introduce CI to keep this from happening in the future at the same time, that might be worth thinking about.

But in its current state it will just make git blame less useful and then inevitably get un-sorted again after a while.

@timokau
Copy link
Member

timokau commented Jan 23, 2020

Since this has stalled for a week and most people seem to be on the "not worth it" side, do you agree that we can close this @yorickvP?

@yorickvP
Copy link
Contributor Author

CI for not making it worse seems nontrivial. Closing for now.

@yorickvP yorickvP closed this Jan 24, 2020
@yorickvP yorickvP deleted the sort-maintainers-list branch January 24, 2020 11:08
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/please-fix-all-packages-which-were-broken-by-the-qt-wrapping-changes/6444/17

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

8 participants