potiuk commented on code in PR #36472: URL: https://github.com/apache/airflow/pull/36472#discussion_r1437831676
########## CONTRIBUTING.rst: ########## @@ -464,9 +487,97 @@ Step 5: Pass PR Review Note that committers will use **Squash and Merge** instead of **Rebase and Merge** when merging PRs and your commit will be squashed to single commit. -You need to have review of at least one committer (if you are committer yourself, it has to be +When the maintainer starts conversations it is expected that you respond to questions, suggestions, doubts, +and generally it's great if all such conversations seem to converge to a common understanding. You do not +necessarily have to apply all the suggestions (often they are just opinions and suggestions even if they are +coming from seasoned maintainers) - it's perfectly ok that you respond to it with your own opinions and +understanding of the problem and your approach and if you have good arguments, presenting them is a good idea. + +The reviewers might leave several types of responses: + +* ``General PR comment`` - which usually means that there is a question/opinion/suggestion on how the PR can be + improved, or it's an ask to explain how you understand the PR. You can usually quote some parts of such + general comment and respond to it in your comments. Often comments that are raising questions in general + might lead to different discussions, even a request to move the discussion to the devlist or even lead to + completely new PRs created as a spin-off of the discussion. + +* ``Comment/Conversation around specific lines of code`` - such conversation usually flags a potential + improvement, or a potential problem with the code. It's a good idea to respond to such comments and explain + your approach and understanding of the problem. If you do not agree with the comment, you can explain why + you think your approach is better. If you agree with the comment, you can explain why you did not do it + this way in the first place. If you agree with the comment and you think it's a good idea, you can just + apply the suggestion and push the change to your PR. You can also resolve the conversation if you think the + problem raised in the comment or ask the reviewer to re-review, confirm it and resolve the conversation if + you are not sure. If you do not understand the comment, you can ask for clarifications. Generally assume + good intention of the person who is reviewing your code, they are not criticising you, at most they care + about the quality of the code and the project and want to make sure that the code is as good as possible. + + It's ok to mark the conversation resolved by anyone who can do it - it could be the author, that thinks + the arguments are changes implemented make the conversation resolved, or the maintainer/person who + started the conversation or it can be even marked as resolved by the maintainer who attempts to merge the + PR and thinks that all conversations are resolved. However if you want to make sure attention and decision + on merging the PR is given by maintainer, make sure you monitor, follow-up and close the conversations when + you think they are resolved (ideally explaining why you think the conversation is resolved). + +* ``Request changes`` - this is where maintainer is pretty sure that you should make a change to your PR + because it contains serious flaw, design misconception, or a bug or it is just not in-line with the common + approach Airflow community took on the issue. Usually you should respond to such request and either fix + the problem or convince the maintainer that they were wrong (it happens more often than you think). + Sometimes even if you do not agree with the request, it's a good idea to make the change anyway, because + it might be a good idea to follow the common approach in the project. Sometimes it might even happen that + two maintainer will have completely different opinions on the same issue and you will have to lead the Review Comment: yep -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
