These days, the nixpkgs PR template explicitly asks contributors to consider reviewing other PRs, which is something I’ve picked up myself e.g. here.
However, I always feel a little awkward about doing so because it can be hard for the PR author to distinguish between “this random bystander is offering me advice” vs. “this package maintainer is telling me the conditions under which they will accept my contribution”.
Should I always state that I’m not a maintainer or any other kind of gatekeeper in my reviews? Or is there anything else folks can advise me about how best to think about my role, and how best to communicate it?
(Relatedly, but less importantly, I wish there was explicit guidance about whether to use “approve” / “request changes” vs. “comment”, whether non-authors should click “resolve” on review threads, etc.)
If a reviewer said “do X” and the PR did X then resolving it is fine, in ambiguous cases I’d leave it be unless it’s a nitpick (nits aren’t worth addressing). What qualifies as a nit isnit well-defined, but normally if there’s nothing in the manual or other similar PRs that backs up a suggestion, and no technical reasoning was provided, and it appears to be one person’s preference, it’s likely a nit.
Formatting is almost always a nit, assuming the PR author ran nixfmt-rfc-style on their code, and it’s not shockingly unreadable (e.g. no newlines or something extreme like that)
Requesting changes doesn’t do anything if you’re not a committer, so no need to worry about it.
Though as a social signal, requesting changes on a non-blocking suggestion makes little sense.
I forgot, there’s some differentiation at the right of the name of any review/commenter. You get “Contributor” and e.g. the author of that PR gets “Member”.
IMO it doesn’t matter whether you have commit privileges or not. If someone with commit privileges would have done the same review, it’d have been the same outcome.
However, if you’re not very experienced with code review (as in: judging the Nix expression diff) in the context of Nixpkgs, I’d kindly ask you to leave that to the committer team though. This isn’t to gatekeep or because we want to keep that for ourselves to enjoy but rather to minimise the amount of insubstantial/bikeshed-y or even misguided code review. This again isn’t to say inexperienced reviewers always do this or anything but rather that us experienced reviewers are already quite bad enough at this and we don’t need any more than we currently have of that.
What we actually need help with the most is verifying that PRs do what they intend to do and don’t break anything unintentionally. If you see a PR touching a package that you yourself use or know how to use and can test that the package works as expected, that’s 10x more helpful to us than if you did code review on 3 PRs.
Please be aware that opinion on this is unlikely to be uniform in the committer team.
An approved-by label is helpful for people deciding whether to merge the PR, but it doesn’t tell you whether the person currently reviewing your PR is a package maintainer (at least, not before they’ve approved).
A contributor is someone who does not have collaborator access to a repository but has contributed to a project and had a pull request they opened merged into the repository.
[…]
A collaborator is a person with read and write access to a repository who has been invited to contribute by the repository owner.
However, if you ask a random GitHub user what the difference is between a “contributor” and a “collaborator”, I feel not very optimistic they’d know
It’s also still not quite right because I’m not sure if package maintainers are always given write access, and also collaborator status is per-repo, not per-package.
