Review guide

[walterbender]

Let's discuss how to advise reviewers as to best practice.

[vyagh]

• Smoke Test is very important
• Check License headers (when applicable)
• Description should match the Code

[walterbender]

• No unrelated changes (lots of variants of this -- forgot to rebase; linting unrelated files; or PR should be multiple PRs)
• No lint or jest errors

[e-esakman]

• Thoroughly check the folder directory especially for tests
• Also worth checking for common security issues like XSS, unsafe URL handling, inline JS etc.

[farhan-momin]

We can have a few sections under Review guidelines-

• For New contributors
• Reviewing steps
• Do's and dont's/ Best practices

[karthik-dev56]

We could make a Separate ReviewGuidlines.md

1. New Contributors review small fix and suggest changes
2. after certain PRS merge like 8-10 go for approval
3. After certain experience bring up with new Performance Improvements and Features adding and Discuss with Mentors

[farhan-momin]

1 and 3 look good.

About point 2, I think making approval in review is not necessary for contributors. Also approvals cannot be taken back once made (I faced that before 😅).

Maybe we should just ask to write 'LGTM'.

[karthik-dev56]

1 and 3 look good.

About point 2, I think making approval in review is not necessary for contributors. Also approvals cannot be taken back once made (I faced that before 😅).

Maybe we should just ask to write 'LGTM'.
Its should be necessary bcz we manually review the code and come to conlusion assuring that 80 to 90% code is correct later maintener looks that Approval is sign of code acceptance that helps contributors manually read and accepts

[zealot-zew]

I have been utilizing the test suite to evaluate some PRs, As this would help ensure that new changes do not introduce regressions or break existing functionality

[walterbender]

Yes. I use that when there are block behavior changes in the PR.