Open chunfuwen opened 4 years ago
sathnaga
commented
4 years ago All good points..
Additionally, we need to define the threshold for closing a PR if not attended after the review. e:g:- say no response from contributor after 1 month, the PR will be labelled as, "No response" and after 3 months it will be closed, something like that?
pevogam
commented
3 years ago I want to add a concern that I raised in https://github.com/avocado-framework/avocado-vt/pull/2413#issuecomment-696227408 since it relates very well here and the reality of some areas of code in avocado. This is an excerpt of it:
In particular, there are some very weakly covered areas of Avocado VT in terms of usage where there are <2 people that actually use the functionality. The requirement of at least 2 reviews could then never be met. I recently closed a PR that waited more than 6 months for a second person to set their eyes on it (incl. of people I pinged as @luckyh suggested they might be using the functionality it concerned). Is there a way to remedy this larger problem of the Avocado VT project? I have been plagued by this problem for years now as very often it turned out we use functionality that was last used in Autotest and is now still present but not touched by anyone.
What are you thoughts about this?
Libvirt Team members summarize some rules or guidelines related to Upstream PR Review/Contribute as per past practices. So it may be good to open for public discussion in order to get more feedback, eventually consider whether it is possible to become common best practices in avocado-vt upstream.
For reviewers rules:
Everyone who has experiences in the project is encouraged to review PRs. Respectful, kind, patient to the coders Freely deny for changes the codebase does not want/need even though perfect design/codes Ask questions rather than make statements. 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 ...? Remember to praise. Remember that there is often more than one way to approach a solution. Given clear and useful comments. 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) Share your best practice/knowledge as a mentor. Cautiously regard personal preference as best practice and impose to contributors Dismiss your approval if you add new comment for a PR after you already have given an approval It is ok to use a 'request review' tool to ask someone to review as you like, but not a must. It is ok to cancel the request review to you if you think you are not a suitable one for this PR. Wait for request review if there is for 2 weeks for those PRs which have already 2 approvals Give a more clear comment with your reason to change.
Maintainer rules:
includes all reviewer's rules Make sure all PRs submitted can be closed within 3 months 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' Mark proper and necessary labels according to the information provided by the PR contributor
Contributors Rules:
[Must] Coding style compliance, for example, align with inspekt tool, pep8
[Must] PR commit message is meaningful. Refer to the link on how to write a good commit message [Must] Travis CI pass and no conflict [Must] Provide test results. If no, provide justification. Apply to any PR One of below options should be aligned: [Must] If the function defined with right docstring (description and params, and return if have) [Must] If the PR depends on other PRs, ensure the PR is merged after other PRs [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. [Must] If the case does some package version judgement for the new case support or compatible backwards [Must] If the test code have suitable env backup and recovery steps [Optional] If have the necessary and clear comments for the code explanation for steps [Optional] If the case is applied to multiple arches. [Optional] If have duplication, need to create new function or reuse existing library in avocado/avocado-vt [Optional] If the logic are complete and no important branches which are not dealt with [Optional] If the code seems clear and concise, define functions to increase the readability [Optional] Use python supported library instead of shell cmd running by process.run if possible [Optional] If the feature test related aspects are correct [Optional] Add comments to ask questions which you do not understand [Optional] Pay more attention to ‘test.fail(xxx)’ or exception raise part, such as if there is log info [Must] Add a comment to say if your PR has a dependence [Optional] Reply to the comment when you have fixed the comment (see good sample in Appendix.4) [Optional] Better to use @someone to ask for review when your PR is submitted [Optional] Use ‘request review’ to ask the original reviewer to request again when you finish updates