-
-
Notifications
You must be signed in to change notification settings - Fork 634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify how to fill the code Review Checklist: #12054
Comments
Thanks for raising this @CyrilleB79. If you haven't already, could you please read https://github.com/nvaccess/nvda/wiki/Github-pull-request-template-explanation-and-examples This explanation may need further clarification. The aim of this checklist is to ensure each item has been considered by both the author and the reviewer. Hopefully it helps to prevent items being forgotten. The reviewer and author need to use their best judgement on whether they think further changes need to be made after reviewing the checklist and if so, they can start a conversation about it. Its ok that not all items will be applicable for all situations. Checking the item lets us know its been considered. Perhaps I should add this explanation to the wiki page, what do you think? |
First of all, sorry, I had not read the updated wiki PR explanation page. Now it's done. I find such a check list very useful for PR submitters as well as reviewers. Thanks. However, my understanding is that all the checkboxes should be checked since all these points need to have been considered, even if not applicable. Am I correct? In this case, I would find more useful to have an explicit Yes / No answer to each point, indicating if this point is applicable or not to the current PR. No answer would indicate that the specific point has not been considered, which is not recommended. |
Yes. I only intend it to serve as a reminder, not a list of requirements and not a declaration. For most of these items there is space in the PR template to talk about the specifics, I'd prefer long form explanation rather than a simple 'yes'. In the end, the proof is the diff / PR description, reviewers still need to go and look at these things. Mostly, I intended this list for the reviewer, but I think it is a good idea to make the reviewers expectations clear to the author also. Authors can leave these items unchecked before a PR is ready for a review. I'll look at rewording the template and wiki to clarify. I'd like to give the current approach some time, but feedback is certainly welcome. |
It is not very clear how to fill the new Code Review Checklist.
Steps to reproduce:
Actual behavior:
It is not clear when an item should be checked. Also maybe a comment to indicate how to check these check boxes is needed. We can already see in the few new PRs that people seem to use this list diversely according to situations:
Should unit test be checked if the PR was unit tested or if a unit test was added? Should help context be checked only if a new GUI item was added or also if I have checked that my PR does not need to implement help context.
More generally:
Expected behavior:
System configuration
NVDA installed/portable/running from source:
source
NVDA version:
Last alpha
Windows version:
N/A
Name and version of other software in use when reproducing the issue:
N/A
Other information about your system:
Other questions
Does the issue still occur after restarting your computer?
N/A
Have you tried any other versions of NVDA? If so, please report their behaviors.
N/A
If addons are disabled, is your problem still occuring?
N/A
Did you try to run the COM registry fixing tool in NVDA menu / tools?
N/A
The text was updated successfully, but these errors were encountered: