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

yorickvP commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Contributor Author

yorickvP commented Jan 15, 2020

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

@yorickvP yorickvP force-pushed the yorickvP:sort-maintainers-list branch from 53ab7d6 to 58631c4 Jan 15, 2020
@yorickvP

This comment has been minimized.

Copy link
Contributor Author

yorickvP commented Jan 15, 2020

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

@Infinisil

This comment has been minimized.

Copy link
Member

Infinisil commented Jan 15, 2020

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

alyssais 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.

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

This comment has been minimized.

Copy link
Member

7c6f434c commented Jan 16, 2020

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

@alyssais

This comment has been minimized.

Copy link
Member

alyssais commented Jan 16, 2020

@7c6f434c

This comment has been minimized.

Copy link
Member

7c6f434c commented Jan 17, 2020

@timokau

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor Author

yorickvP commented Jan 24, 2020

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

@yorickvP yorickvP closed this Jan 24, 2020
@yorickvP yorickvP deleted the yorickvP:sort-maintainers-list branch Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.