From 020bcb0c64715a036440e26ddef405fcaab39904 Mon Sep 17 00:00:00 2001 From: Cleber Rosa Date: Thu, 20 Jan 2022 13:29:49 -0500 Subject: [PATCH] docs/contributing: give more visibility to review and contribute doc There's a very good document on how to proceed with reviews, maintainance and overall contributions that has lower visibility because it's a standalone text document. This moves the content to the documentation proper so that more people are aware of it. Signed-off-by: Cleber Rosa --- PR_Review_And_Contribute_Practices.txt | 60 ---------------------- docs/source/contributing/Guidelines.rst | 67 +++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 60 deletions(-) delete mode 100644 PR_Review_And_Contribute_Practices.txt diff --git a/PR_Review_And_Contribute_Practices.txt b/PR_Review_And_Contribute_Practices.txt deleted file mode 100644 index ab5413ee82..0000000000 --- a/PR_Review_And_Contribute_Practices.txt +++ /dev/null @@ -1,60 +0,0 @@ -For reviewers rules: -============================================================================ - -1)Everyone who has experiences in the project is encouraged to review PRs. -2)Respectful, kind, patient to the coders -3)Freely deny for changes the codebase does not want/need even though perfect design/codes -4)Ask questions rather than make statements. -5)Not encourage the “Why” questions. - Good practice: e.g. Wouldn't it make more sense to - Would you like to? Could you give the reason that ...? -6)Remember to praise. -7)Remember that there is often more than one way to approach a solution. -8)Given clear and useful comments, and explain the reason why reqest change -9)In general, reviewers should favor approving a PR once it is in a state where it definitely improves the overall code health of the system being worked on, - even if the PR isn't perfect.(maintainability, readability, and understandability) -10)Share your best practice/knowledge as a mentor. -11)Cautiously regard personal preference as best practice and impose to contributors -12)Dismiss your approval if you add new comment for a PR after you already have given an approval -13)It is ok to use a 'request review' tool to ask someone to review as you like, but not a must. -14)It is ok to cancel the request review to you if you think you are not a suitable one for this PR. -15)Wait for request review for no more than 2 weeks on those PRs which have already 2 approvals - - -Maintainer rules: -============================================================================= - -1)includes all reviewer's rules -2)Make sure all PRs submitted can be closed within 3 months -3)Generally every PR needs at least 1 maintainer’s approval and total 2 approvals before being merged. - Add request for review for more maintainers if there is a need - Add a comment to explain why a PR need the label 'request_2_maintainers' -4)Mark proper and necessary labels according to the information provided by the PR contributor -5)Closing a PR threshold:no response from contributor after 1 month, the PR will be labelled as, "No response" and after 3 months it will be closed - - -Contributors Rules: -============================================================================ - -1)[Must] Coding style compliance, for example, align with inspekt tool, pep8 -2)[Must] PR commit message is meaningful. Refer to the link on how to write a good commit message -3)[Must] Travis CI pass and no conflict -4)[Must] Provide test results. If no, provide justification. Apply to any PR - One of below options should be aligned: -5)[Must] If the function defined with right docstring (description and params, and return if have) -6)[Must] If the PR depends on other PRs, please add a comment to say if your PR has a dependence in order to ensure the PR is merged after dependence PRs -7)[Must] If the API of one library is changed, ask for all test cases to be modified which invoke this library and provide test results of representative test cases. -8)[Must] If the case does some package version judgement for the new case support or compatible backwards -9)[Must] If the test code have suitable env backup and recovery steps -10)[Optional] If have the necessary and clear comments for the code explanation for steps -11)[Optional] If the case is applied to multiple arches. -12)[Optional] If have duplication, need to create new function or reuse existing library in avocado/avocado-vt -13)[Optional] If the logic are complete and no important branches which are not dealt with -14)[Optional] If the code seems clear and concise, define functions to increase the readability -15)[Optional] Use python supported library instead of shell cmd running by process.run if possible -16)[Optional] If the feature test related aspects are correct -17)[Optional] Add comments to ask questions which you do not understand -18)[Optional] Pay more attention to ‘test.fail(xxx)’ or exception raise part, such as if there is log info -19)[Optional] Reply to the comment when you have fixed the comment (see good sample in Appendix.4) -20)[Optional] Better to use @Someone to ask for review when your PR is submitted -21)[Optional] Use ‘request review’ to ask the original reviewer to request again when you finish updates diff --git a/docs/source/contributing/Guidelines.rst b/docs/source/contributing/Guidelines.rst index 9b1cd1e80c..995455764b 100644 --- a/docs/source/contributing/Guidelines.rst +++ b/docs/source/contributing/Guidelines.rst @@ -32,3 +32,70 @@ extra language. For example in American English, `"Rinse and repeat" `_ is a funny phrase, but could cause problems when translated into other languages. Basically, try to avoid anything that slows the reader down from finding facts. + +Rules for Reviewers +=================== + +1. Everyone who has experiences in the project is encouraged to review PRs. +2. Respectful, kind, patient to the coders +3. Freely deny for changes the codebase does not want/need even though + perfect design/codes +4. Ask questions rather than make statements. +5. Not encourage the “Why” questions. Good practice: e.g. Wouldn't it make more sense to + Would you like to? Could you give the reason that ...? +6. Remember to praise. +7. Remember that there is often more than one way to approach a solution. +8. Given clear and useful comments, and explain the reason why reqest change +9. In general, reviewers should favor approving a PR once it is in a + state where it definitely improves the overall code health of the + system being worked on, even if the PR isn't perfect + (maintainability, readability, and understandability). +10. Share your best practice/knowledge as a mentor. +11. Cautiously regard personal preference as best practice and impose + to contributors +12. Dismiss your approval if you add new comment for a PR after you + already have given an approval +13. It is ok to use a 'request review' tool to ask someone to review + as you like, but not a must. +14. It is ok to cancel the request review to you if you think you are + not a suitable one for this PR. +15. Wait for request review for no more than 2 weeks on those PRs + which have already 2 approvals + +Rules for Maintainers +===================== + +1. Includes all reviewer's rules +2. Make sure all PRs submitted can be closed within 3 months +3. Generally every PR needs at least 1 maintainer’s approval and total + 2 approvals before being merged. + a) Add request for review for more maintainers if there is a need + b) Add a comment to explain why a PR need the label 'request_2_maintainers' +4. Mark proper and necessary labels according to the information provided by the PR contributor +5. Closing a PR threshold:no response from contributor after 1 month, the PR will be labelled as, "No response" and after 3 months it will be closed + +Rules for Contributors +====================== + +1. [Must] Coding style compliance, for example, align with inspekt tool, pep8 +2. [Must] PR commit message is meaningful. Refer to the link on how to write a good commit message +3. [Must] Travis CI pass and no conflict +4. [Must] Provide test results. If no, provide justification. Apply to any PR + One of below options should be aligned: +5. [Must] If the function defined with right docstring (description and params, and return if have) +6. [Must] If the PR depends on other PRs, please add a comment to say if your PR has a dependence in order to ensure the PR is merged after dependence PRs +7. [Must] If the API of one library is changed, ask for all test cases to be modified which invoke this library and provide test results of representative test cases. +8. [Must] If the case does some package version judgement for the new case support or compatible backwards +9. [Must] If the test code have suitable env backup and recovery steps +10. [Optional] If have the necessary and clear comments for the code explanation for steps +11. [Optional] If the case is applied to multiple arches. +12. [Optional] If have duplication, need to create new function or reuse existing library in avocado/avocado-vt +13. [Optional] If the logic are complete and no important branches which are not dealt with +14. [Optional] If the code seems clear and concise, define functions to increase the readability +15. [Optional] Use python supported library instead of shell cmd running by process.run if possible +16. [Optional] If the feature test related aspects are correct +17. [Optional] Add comments to ask questions which you do not understand +18. [Optional] Pay more attention to ‘test.fail(xxx)’ or exception raise part, such as if there is log info +19. [Optional] Reply to the comment when you have fixed the comment (see good sample in Appendix.4) +20. [Optional] Better to use @Someone to ask for review when your PR is submitted +21. [Optional] Use ‘request review’ to ask the original reviewer to request again when you finish updates