Stream: t-compiler/wg-rls-2.0

Topic: Reviewing the review process


matklad (Jun 02 2020 at 09:18, on Zulip):

Hey, so I feel we are suffering from a review bandwidth deficit. I want to help with this, by documenting more of the "internal invariants" and "how to do review" stuff.

I am putting info here: https://github.com/rust-analyzer/rust-analyzer/pull/4703

matklad (Jun 08 2020 at 11:04, on Zulip):

Question: how do folks feel about reviewers pushing commits directly to the PR branch (allow edits from maintainers)?

I've noticed that the way I do thorough review is by checking out the code locally, and testing if my suggestions compile. So, in a sense, I first apply suggestions locally, and then write it again, as the comment on the PR.

I wonder if it makes sense to just directly push the suggestsions (preserving the same "motivation" part I would to GH comment to explain why this change is a good idea in my options)

vsrs (Jun 08 2020 at 11:50, on Zulip):

LGTM

Jeremy Kolb (Jun 08 2020 at 13:16, on Zulip):

I think it should be allowed but would also say that I've learned a lot of rust from you telling me my PRs are wrong and having to type it all out myself :)

matklad (Jun 08 2020 at 13:54, on Zulip):

Yeah, I guess, that's the main question -- would pedagogical value of reading someone else's commit be comparable to typing the stuff yourself...

bjorn3 (Jun 08 2020 at 14:05, on Zulip):

When you type something yourself, you are forced to consciously think about it. When you read a commit you might just skim it without paying too much attention to the actual changes.

std::Veetaha (Jun 10 2020 at 01:42, on Zulip):

I'd go for code snippet suggestions in PR itself. Pushing directly to PR branch you don't leave space for objections

Joshua Nelson (Jun 10 2020 at 01:44, on Zulip):

Code snippets are nice but I find that I'm tempted to make suggestions without testing them

Joshua Nelson (Jun 10 2020 at 01:45, on Zulip):

which I guess could be good pedagogically :laughing: but also means I could suggest something that fundamentally doesn't work

Last update: Sep 27 2020 at 14:15UTC