maintainers: prefix number with underscore #95553

zowoq · 2020-08-16T03:11:48Z

This is the same pattern that is already used for packages, I think it makes sense to reuse it here.

Lines 531 to 545 in 4789d6f

    
           _0x0 = callPackage ../tools/misc/0x0 { }; 
        
           _3llo = callPackage ../tools/misc/3llo { }; 
        
           _3mux = callPackage ../tools/misc/3mux { }; 
        
           _1password = callPackage ../applications/misc/1password { }; 
        
           _1password-gui = callPackage ../tools/security/1password-gui { 
        
             electron = electron_9; 
        
           }; 
        
           _6tunnel = callPackage ../tools/networking/6tunnel { }; 
        
           _9pfs = callPackage ../tools/filesystems/9pfs { };

GitHub doesn't allow usernames with underscores so these won't collide with another user.

samueldr

I don't think this is necessary. Though, other Nixpkgs maintainers can agree or disagree, and my judgement shouldn't be a blocker.

The reason it is required for packages is that on the command-line, --attr can't work with leading numbers:

~/tmp/nixpkgs/nixpkgs 1 $ nix-build -A 0x0
error: syntax error, unexpected INT, at /Users/samuel/tmp/nixpkgs/nixpkgs/pkgs/top-level/all-packages.nix:531:3

(You can modify all-packages.nix to provide a 0x0 if you want, but the result is the same if it isn't there.)

I don't think we're expected to interact with maintainers using --attr; at the time we will, this could be re-considered. After all, this is a relatively minor mechanical change, so hopefully this is a triviality.

In addition, handle is not forcibly the same as a GitHub alias. It is possible a user uses Nixpkgs without a GitHub account, even contributes without a GitHub account (yes, technically things are in place for that), and has a prefixed underscore for their handle. In that instance it becomes a bit annoying :) Though we're erring in the world of aggravating edge cases, so don't mind that last point too much.

Should have read the other discussion

samueldr · 2020-08-16T03:24:46Z

Sorry, now I see why this is relevant. And this is using an existing precedent.

jonringer · 2020-08-16T04:05:41Z

cc @0x4A6F @1000101

mmahut · 2020-08-16T10:00:59Z

Oh, not this again, I feel like I spend hours on discussing this trivial case. I don't think
this is a solution, unless it is agreed as a standard (which is in packages names, because there is a real
technical reason for this, not that someone is not paying and testing their changes introducing introduces a bug).

But if feel like this is a trip wire, let's create a meta data check in ofborg, that will be beneficial in the future as well.

zowoq · 2020-08-16T10:08:28Z

let's create a meta data check in ofborg

Not exactly easy to actually get it done and we might end up moving some checks away from ofborg anyway.

1000101

Fine by me. It's been fun, but became very tiring eventually. It pisses off too many people and since there is no simple solution, it's not worth arguing over.

Let's change it and, ideally, document the need for an underscore somewhere - maybe here?

FRidh · 2020-08-17T12:33:39Z

I suggest also mapping over the keys and asserting none start with a number.

0x4A6F

Please use the common pattern with maintainers; [ _0x4A6F ];.

zowoq · 2020-08-17T22:04:21Z

I suggest also mapping over the keys and asserting none start with a number.

Can you elaborate please?

jonringer · 2020-08-17T22:33:30Z

I'm assuming having CI do something to the effect of:

  lib.mapAttrs (name: value:  assert (builtins.match "^([0-9]).*") name == null; value) lib.maintainers

where there's a check that a new maintainer doesn't start with a number.

zowoq · 2020-08-17T23:28:45Z

I don't think adding it to ofborg is a productive use of time as there has been some discussions about moving things away from ofborg. Bus factor is also an issue so I don't want to add more things there anyway.

I don't want to add it as a check that runs on hydra as that depends on someone noticing and caring about it enough to unbreak it, I think we have enough things already that we don't know are broken until after a PR is merged (e.g. the manuals).

I'm happy to implement this and other maintainer checks (e.g. check github has a matching githubId) via an action that actually catches the errors before merge, when we can actually use actions.

jonringer · 2020-08-18T00:33:37Z

I'm happy to implement this and other maintainer checks (e.g. check github has a matching githubId) via an action that actually catches the errors before merge, when we can actually use actions.

I think this would be easy enough to implement as a github action. But I'm not sure what we want to do as a community.

zowoq · 2020-08-18T05:10:51Z

Just to be clear about the current status of actions: we can't use them at the moment.

We were using two actions but they were removed because they caused too much disruption. We're now waiting for a couple of things to be resolved before we try them again, I don't really have a timeframe for when that will happen.

I'd want to have those actions running for a couple of weeks without causing any problems before we start adding more.

I see this PR as a slight improvement over the status quo even without a way of enforcing it, I don't think having a check should be a blocker.

FRidh · 2020-08-18T06:11:56Z

I am not suggesting an explicit test, but having the map @jonringer wrote directly in the maintainers file. Eval errors are noticed.

jonringer · 2020-08-18T06:42:21Z

how did you want to expose this? doing the evaluation on just lib.maintainers would mean evaluating all 1000+ entries any time you want to access a given maintainer. Something like lib.maintainers._verifyNames? Couldn't think of any good names

FRidh · 2020-08-18T06:53:23Z

Actually, lib/tests/release.nix is already executed by ofborg and that includes lib/tests/maintainers.nix. That will be cheaper than directly in maintainer-list.nix.

zowoq · 2020-08-18T08:07:50Z

Sorry but I'm not going to implement what has been requested.

Should I close this PR?

jonringer · 2020-08-18T08:13:31Z

I think the PR is fine as is.

The check can happen in a different PR

1000101 · 2020-08-18T08:51:59Z

Sorry but I'm not going to implement what has been requested.

Should I close this PR?

Don't you dare! 😄

zowoq requested review from FRidh and jonringer as code owners August 16, 2020 03:11

zowoq mentioned this pull request Aug 16, 2020

maintainers: rename 1000101 to b1000101 #90029

Closed

10 tasks

ofborg bot added 6.topic: nixos 6.topic: python 8.has: module (update) labels Aug 16, 2020

samueldr previously requested changes Aug 16, 2020

View reviewed changes

ofborg bot added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Aug 16, 2020

FRidh approved these changes Aug 16, 2020

View reviewed changes

worldofpeace approved these changes Aug 17, 2020

View reviewed changes

1000101 suggested changes Aug 17, 2020

View reviewed changes

1000101 approved these changes Aug 17, 2020

View reviewed changes

0x4A6F requested changes Aug 17, 2020

View reviewed changes

zowoq added 3 commits August 18, 2020 07:59

maintainers: 0x4A6F -> _0x4A6F

7d9c49f

maintainers: 1000101 -> _1000101

0052523

maintainers: add note about underscore prefix for handles

17f765b

maintainers: remove unneeded quotes from handles

83a1f0a

0x4A6F approved these changes Aug 18, 2020

View reviewed changes

jonringer approved these changes Aug 18, 2020

View reviewed changes

FRidh merged commit fe7bab3 into NixOS:master Aug 18, 2020

zowoq deleted the rename-maintainers branch August 18, 2020 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

maintainers: prefix number with underscore #95553

maintainers: prefix number with underscore #95553

zowoq commented Aug 16, 2020

samueldr left a comment

samueldr commented Aug 16, 2020

jonringer commented Aug 16, 2020

mmahut commented Aug 16, 2020

zowoq commented Aug 16, 2020

1000101 left a comment

FRidh commented Aug 17, 2020

0x4A6F left a comment

zowoq commented Aug 17, 2020

jonringer commented Aug 17, 2020 •

edited

zowoq commented Aug 17, 2020

jonringer commented Aug 18, 2020 •

edited

zowoq commented Aug 18, 2020

FRidh commented Aug 18, 2020

jonringer commented Aug 18, 2020

FRidh commented Aug 18, 2020

zowoq commented Aug 18, 2020

jonringer commented Aug 18, 2020

1000101 commented Aug 18, 2020

	_0x0 = callPackage ../tools/misc/0x0 { };

	_3llo = callPackage ../tools/misc/3llo { };

	_3mux = callPackage ../tools/misc/3mux { };

	_1password = callPackage ../applications/misc/1password { };

	_1password-gui = callPackage ../tools/security/1password-gui {
	electron = electron_9;
	};

	_6tunnel = callPackage ../tools/networking/6tunnel { };

	_9pfs = callPackage ../tools/filesystems/9pfs { };

maintainers: prefix number with underscore #95553

maintainers: prefix number with underscore #95553

Conversation

zowoq commented Aug 16, 2020

samueldr left a comment

Choose a reason for hiding this comment

samueldr commented Aug 16, 2020

jonringer commented Aug 16, 2020

mmahut commented Aug 16, 2020

zowoq commented Aug 16, 2020

1000101 left a comment

Choose a reason for hiding this comment

FRidh commented Aug 17, 2020

0x4A6F left a comment

Choose a reason for hiding this comment

zowoq commented Aug 17, 2020

jonringer commented Aug 17, 2020 • edited

zowoq commented Aug 17, 2020

jonringer commented Aug 18, 2020 • edited

zowoq commented Aug 18, 2020

FRidh commented Aug 18, 2020

jonringer commented Aug 18, 2020

FRidh commented Aug 18, 2020

zowoq commented Aug 18, 2020

jonringer commented Aug 18, 2020

1000101 commented Aug 18, 2020

jonringer commented Aug 17, 2020 •

edited

jonringer commented Aug 18, 2020 •

edited