I don't feel familiar enough with the pnpm codebase to review this, but if there were documentation added I could review that.

I don't feel familiar enough with the pnpm codebase to review this

There’s only really this logic to go over here, which is not particularly entangled with the rest of the code. @zkochan might like some changes, but I doubt anything drastically different to what I sketched out there, other than maybe potentially generalising checkPackageManager() and reusing that…

if there were documentation added I could review that.

Yeah that’ll be the last step, after approval.

Yeah that’ll be the last step, after approval.

Maybe this is my bias coming from other projects, but generally what I'm used to is starting with documentation, and reaching agreement on that, so that everyone is aligned on what the intent of the PR is. Then the code review is just about whether or not there are bugs and whether it correctly implements the intent as described in the documentation.

Yeah that’ll be the last step, after approval.

Maybe this is my bias coming from other projects, but generally what I'm used to is starting with documentation, and reaching agreement on that, so that everyone is aligned on what the intent of the PR is. Then the code review is just about whether or not there are bugs and whether it correctly implements the intent as described in the documentation.

That’s a fine approach, and one Iv’e taken at times, but in this particular case we kind of already have that in the form of your existing spec, no?

covered by #10932