I think emphasizing pulling the code and running it as a pre-req to engaging in a code review is increasing the friction of the code review sufficiently that you're going to get less engagement on a given PR across the team. This seems bad to me.
In my opinion, an ideal PR gets lots of contact across the team and robust conversation around proposed changes. A high quality PR should include automated tests to validate the work, and sure, there should be some manual verification of what was done, but I think the author can own most of this.
I think emphasizing pulling the code and running it as a pre-req to engaging in a code review is increasing the friction of the code review sufficiently that you're going to get less engagement on a given PR across the team.
Potentially, though my experience is the opposite. I'm more engaging after I pull down code and poke at it in my IDE and try to run it than just commenting on a webpage.
Also, I wonder: is engagement on a pull request the right thing to optimize for? Shouldn't we optimize for the best solution or the best code?
I'm more engaging after I pull down code and poke at it in my IDE and try to run it than just commenting on a webpage.
Yeah, I would agree the depth of engagement is higher when you give it that level of attention. And I definitely would prefer my teammates to dig deep and run the code, especially on particularly complex code changes.
Also, I wonder: is engagement on a pull request the right thing to optimize for? Shouldn't we optimize for the best solution or the best code?
That's a great question. I don't think it's the only the thing you optimize for. But I think it's generally better to get broad exposure, especially on significant changes. This doesn't only benefit the quality of code going into the codebase, but also is an opportunity for the team to keep up to date on the latest changes and to engage in discussion and learn from each other.
Oftentimes, I've been on projects where reviewing the PR means that you, as reviewer, are now also tapped for questions and follow up work on that bit of code. Later, when we look at git blame (terribly named) for further development, the reviewer listed is someone who may get tagged in.
So, you should run the code, because you might end up having to pick up the baton to fix or extend it next. Ideally, there is no single author of the code base. A PR is often a good point for spreading the responsibility and know-how. And spending some time as a reviewer to manually verify functionality can often catch things the bleary-eyed author missed. Even assumptions in automated tests can be wrong.
Another aspect: There's a such thing as too much engagement on a PR. Not everything needs the whole team dog-piling on it. Not everyone needs to feel required to robustly converse about every change. Most PRs, IMO, should be small enough for just one other set of eyes. (Not that I've always managed to adhere to that)
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
I think emphasizing pulling the code and running it as a pre-req to engaging in a code review is increasing the friction of the code review sufficiently that you're going to get less engagement on a given PR across the team. This seems bad to me.
In my opinion, an ideal PR gets lots of contact across the team and robust conversation around proposed changes. A high quality PR should include automated tests to validate the work, and sure, there should be some manual verification of what was done, but I think the author can own most of this.
Potentially, though my experience is the opposite. I'm more engaging after I pull down code and poke at it in my IDE and try to run it than just commenting on a webpage.
Also, I wonder: is engagement on a pull request the right thing to optimize for? Shouldn't we optimize for the best solution or the best code?
Yeah, I would agree the depth of engagement is higher when you give it that level of attention. And I definitely would prefer my teammates to dig deep and run the code, especially on particularly complex code changes.
That's a great question. I don't think it's the only the thing you optimize for. But I think it's generally better to get broad exposure, especially on significant changes. This doesn't only benefit the quality of code going into the codebase, but also is an opportunity for the team to keep up to date on the latest changes and to engage in discussion and learn from each other.
Oftentimes, I've been on projects where reviewing the PR means that you, as reviewer, are now also tapped for questions and follow up work on that bit of code. Later, when we look at
git blame(terribly named) for further development, the reviewer listed is someone who may get tagged in.So, you should run the code, because you might end up having to pick up the baton to fix or extend it next. Ideally, there is no single author of the code base. A PR is often a good point for spreading the responsibility and know-how. And spending some time as a reviewer to manually verify functionality can often catch things the bleary-eyed author missed. Even assumptions in automated tests can be wrong.
Another aspect: There's a such thing as too much engagement on a PR. Not everything needs the whole team dog-piling on it. Not everyone needs to feel required to robustly converse about every change. Most PRs, IMO, should be small enough for just one other set of eyes. (Not that I've always managed to adhere to that)