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

maintainers: rename 1000101 to b1000101 #90029

Closed
wants to merge 1 commit into from
Closed

Conversation

@Mic92
Copy link
Contributor

Mic92 commented Jun 10, 2020

Having a name starting with numbers is a problem because
the common pattern:

with maintainers; [ 1000101 ];

does not work and there have been cases where people dripped over it
including fixes in this PR itself.

Motivation for this change
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.
Having a name starting with numbers is a problem because
the common pattern:

with maintainers; [ 1000101 ];

does not work and there have been cases where people dripped over it
including fixes in this PR itself.
@Mic92 Mic92 requested review from FRidh and jonringer as code owners Jun 10, 2020
@Mic92
Copy link
Contributor Author

Mic92 commented Jun 10, 2020

cc @1000101

@jtojnar
Copy link
Contributor

jtojnar commented Jun 10, 2020

Alternately, we could explicitly use maintainers."1000101".

It also feels like a good reason to support attrsets in meta.maintainers so that we can do meta.maintainers = { inherit (lib.maintainers) "1000101"; };.

@1000101
Copy link
Member

1000101 commented Jun 10, 2020

Alternately, we could explicitly use maintainers."1000101".

It also feels like a good reason to support attrsets in meta.maintainers so that we can do meta.maintainers = { inherit (lib.maintainers) "1000101"; };.

That's how I've been doing it so far. I forgot to fix those 3 instances where I used it incorrectly (2 due to lack of knowledge, 1 that's in tests is totally my fault). Had it on my list, but forgot to open PR - sorry about that.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 10, 2020

Alternately, we could explicitly use maintainers."1000101".

It also feels like a good reason to support attrsets in meta.maintainers so that we can do meta.maintainers = { inherit (lib.maintainers) "1000101"; };.

The problem is that it is easy to get wrong. I had another PR where somebody introduced a bug:

- maintainers = with maintainers; [ maintainers."1000101" ];
+ maintainers = with maintainers; [ "1000101" mdlayher ];

Which was the motivation to create this PR. Do really want to leave this trip wire for something else to fall over?

@jtojnar
Copy link
Contributor

jtojnar commented Jun 10, 2020

Should not meta check pick it up? I find not mangling contributor names preferable, even if we used attrValues { inherit (maintainers) "1000101"; } everywhere a name starting with number appears.

@Mic92
Copy link
Contributor Author

Mic92 commented Jun 10, 2020

It does not; this pr itself also fix three broken instances.

@mmahut
Copy link
Member

mmahut commented Jun 11, 2020

Maybe we should fix the meta check fix instead of this work around?

@zowoq
Copy link
Contributor

zowoq commented Jun 12, 2020

Can we use the same underscore prefix style that is already used by packages?

@1000101 1000101 mentioned this pull request Jun 26, 2020
0 of 10 tasks complete
@1000101
Copy link
Member

1000101 commented Jun 26, 2020

@Mic92 I've opened a PR which should fix my previous mistakes. I'm really sorry for all the trouble I've caused. If you want to fix this by changing my name, then go ahead, but at the same time, I think it's fine to have an edge case (like myself) to uncover weird behaviour. I'm not sure how many people have nick starting with a number, but I bet it's only a matter of time when someone else breaks the system.

So:

  • we will have to make this a policy (no leading numbers allowed) and rename every genius who thinks having a name starting with a number is cool
  • deal with it / propose a solution that would deal with edge cases in a nice manner
  • leave it as it is and keep myself as a living example (once everything's fixed) of how to do it properly
@Mic92
Copy link
Contributor Author

Mic92 commented Aug 1, 2020

Too much bike-shedding here. I give up.

@Mic92 Mic92 closed this Aug 1, 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

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