Add content for reviewing PRs in the CONTRIBUTING.md#36686
Conversation
GiteaBot
added
the
lgtm/need 2
GiteaBot
added
lgtm/need 1
|
Need to highlight this: Use real world cases or examples to answer the review questions. Don't imagine or guess. |
Added at 1b918d8 |
|
What if some people keep submitting low-quality code, or don't answer questions directly don't read code but just argue for non-existing problems? |
|
I don't think it should go into too much detail. Just picture basic review etiquette. |
|
Ridiculous Take a look at what you are saying and what you are doing. #36660 (comment) 
|
|
Errors happen, but it's still correct to document how it should be. |
If you still think it's fine to keep errors happening again and again and again, go ahead, introduce more and more rubbish contents. Maybe you should document that: nobody need to care about correctness, nobody should fix wrong contents. Do whatever you want in this project, no need to take responsibility for your actions. 
|
|
As I see it, commit descriptions are rather unimportant. They will never capture all the discussions and decisions that happened during review. So I suggest we just delete most if not all of the PR description content upon merge, then nothing can be wrong or outdated. Anyone who wants more context should read the PR discussions instead. |
I don’t think so. I frequently use the git blame feature, which shows the last commit for each line of code along with its commit message. It’s very helpful for understanding why a particular change was made. I’d rather rely on that directly in the editor than go through every related PR line by line. |
Then it is more ridiculous. You said "it is very helpful" and you "rely on that directly". But, you never really paid attention to the messages, most of the PR messages merged by you are totally a mess. (I and other maintainers have pointed out the problem not only one time) |
bircni
left a comment
bircni
left a comment
There was a problem hiding this comment.
Ensure the PR summary accurately reflects the changes
|
|
||
| ### Reviewing PRs | ||
|
|
||
| Maintainers are encouraged to review pull requests in areas where they have expertise or particular interest. When reviewing, please keep the following principles in mind: |
There was a problem hiding this comment.
I think its hard to read
The bullets are inconsistent in scope: some address reviewers, one addresses PR authors. Separating them or making the audience explicit per bullet would help.
|
|
||
| - Focus feedback on the issue itself and avoid comments about the contributor's abilities. | ||
|
|
||
| - If you request changes (i.e., block a PR), you must provide a clear rationale and, whenever possible, outline a concrete path to resolution. Reviews that do not include sufficient explanation are not constructive and may be disregarded. |
There was a problem hiding this comment.
Who decides that a Review is disregarded?
Who decides if an explanation is sufficient?
|
|
||
| - Provide actionable feedback. Clearly explain what needs to be changed and why, and distinguish between required changes and optional suggestions. | ||
|
|
||
| - When responding to reviewer questions, use real-world cases or examples and avoid speculation. |
There was a problem hiding this comment.
Good intent to tell PR authors how to respond to reviewer feedback, but the surrounding bullets all address reviewers...
|
|
||
| - When responding to reviewer questions, use real-world cases or examples and avoid speculation. | ||
|
|
||
| - Focus feedback on the issue itself and avoid comments about the contributor's abilities. |
There was a problem hiding this comment.
| - Focus feedback on the issue itself and avoid comments about the contributor's abilities. | |
| - Focus on the code, not the person. (be friendly 😄 ) |
There was a problem hiding this comment.
Wouldn't add emojis and I prefer to avoid parens, especially malformed ones like this one (the . should be after parens).
|
|
||
| - If you request changes (i.e., block a PR), you must provide a clear rationale and, whenever possible, outline a concrete path to resolution. Reviews that do not include sufficient explanation are not constructive and may be disregarded. | ||
|
|
||
| - Only approve a PR when you are fully satisfied with its current state. |
There was a problem hiding this comment.
good to tie this strict but we should not have "rubberstamp" approvals which you want to stop by that right?
There was a problem hiding this comment.
Or, clearly declare that this is a rubberstamp, and explain why the the rubberstamp is used.
TBH, rubberstamp is not completely unacceptable to me. Sometimes I also do rubberstamp approval, but I will explain: "I haven't test", or "looks good but I don't need/understand it"
Also, I think it's better suggest authors and approvers to keep eyes on related bug reports in the future, indeed, if a bug is related to a PR, the authors and approvers should know more (and be responsible)
There was a problem hiding this comment.
This repo could not work without rubberstamp reviews, they are essential to get stuff moving because most reviewers don't care enough to give every PR a in-depth review and that's absolutely fine imho.
There was a problem hiding this comment.
So I am the only poor man to help to rewrite every problematic PR.
There was a problem hiding this comment.
I think I should only put a "blocked" label from now on, unless the author can completely make it right, I won't spend any time on helping writing code.
There was a problem hiding this comment.
I wouldn't be overly strict on every PR. If it works, looks reasonable and does not cause regression, I approve it. Wasting time into every PR just because it isn't absolutely perfect is counter-productive imho.
There was a problem hiding this comment.
It's less about being perfect and more about being decent enough. Someone has to keep the mess maintainable so 10 PRs down the line we won't have to redo the feature entirely with migrations in DB since someone designed it badly at the start and it looked okay at the time with no forward thinking.
On top of that, just because something looks good and seems not to break anything does not mean it won't. It's the reviewers task to catch it before it lands as it's usually not the PR author who worked in the repo for a long time and understands how components cross and interact.
I don't find rubberstamps good as it makes the reviews feel like a waste of time - in the end author has no idea if the code is good, reviewer looked and deemed it fine so ok from me and bad permissions check on line 44 is excited to land on main.
The only way rubberstamping can work well IMO is with gerrit system where you can sort of indicate level of review you did.
Are they avoidable here? Probably not, I did +1 ones too few times but a comment won't hurt (which I'll do from now).
There was a problem hiding this comment.
redo the feature entirely with migrations in DB since someone designed it badly
Yes, "Auto accept maintainer edit" is such a misdesigned feature. Should have been enabled from the start. Now only a migration can fix it but we are too afraid of changing that for existing repos.
I don't find rubberstamps good
There's just not enough motivated reviewers to do without rubberstamps. If we had 10 active reviewers, sure, every PR could get insightful reviews and at some point one can trust a regular maintainer to not produce crap.
GiteaBot
added
lgtm/blocked
6 participants
No description provided.