Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Establish a PR review process #5

Open
andrew-chang-dewitt opened this issue Mar 14, 2021 · 2 comments · May be fixed by #6
Open

Establish a PR review process #5

andrew-chang-dewitt opened this issue Mar 14, 2021 · 2 comments · May be fixed by #6
Assignees
@andrew-chang-dewitt @CrackerJack625
Labels
discussion documentation Improvements or additions to documentation

Comments

@andrew-chang-dewitt
Copy link
Contributor

We should formalize a set of criteria that need to be checked in every PR. Let’s shoot for something simple and concise that relies on automated testing in CI/CD where possible.

Let’s start by just throwing suggestions for things that we should look for when reviewing a PR in comments below, then go from there.
@andrew-chang-dewitt andrew-chang-dewitt added documentation Improvements or additions to documentation discussion labels Mar 14, 2021
@andrew-chang-dewitt andrew-chang-dewitt linked a pull request Mar 14, 2021 that will close this issue
Draft
@andrew-chang-dewitt
Copy link
Contributor Author

Some things I think we need to make sure we look at in every PR:

  • Is there a CI process?
    • If so, do all the checks pass?
    • If not, why isn't there one?
    • If not, do all the tests pass when ran locally on latest version?
  • Are the API integration tests complete?
    • Do they cover every API route?
    • Are all of the common cases covered?
    • Are the obvious edge cases covered (invalid input, missing input, etc.)?
  • Are there unit tests?
    • If so, are the necessary parts of the service unit tested (functional layers, not imperative)?
    • If not, why not?
  • Are all the service contracts met?
  • Are all the best practices followed, where applicable & reasonable?
  • Are there any code smells or other weirdness that should be explained better or possibly re-worked by the PR author?

@CrackerJack625 any thoughts?

@andrew-chang-dewitt
Copy link
Contributor Author

When we have a final review checklist, I'll write it up into a document to save on this meta repo so we can use it anytime we review a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andrew-chang-dewitt andrew-chang-dewitt

@CrackerJack625 CrackerJack625

Labels
discussion documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Create PR Review process cheese-drawer/meta
2 participants
@andrew-chang-dewitt @CrackerJack625