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.

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

Code Review Guidelines #1

Merged
merged 2 commits into from
May 13, 2021
Merged

Code Review Guidelines #1

merged 2 commits into from
May 13, 2021

Conversation

marbetschar
Copy link
Member

This PR is an initial draft for providing some guidance for code reviews. It should summarize best practices, philosophies etc. which turned out to be working well for elementary and lower the barrier for people who would like to do code reviews but don't know where to begin. Hopefully such guidelines also lead to easier reviewable pull requests.

Ultimately the goal is to make code review experience more fun/rewarding for developers and reviewers which in turn produces more reviews in a shorter time frame.

Please note I'm not sure if the HIG is the best location to add this kind of documentation, but since the elementeary/docs is focused on "Documentation for 3rd-party app development" the HIG seemed to be the most appropriate place to kick things off.

@marbetschar
Copy link
Member Author

as @jeremypw mentioned in elementary/hig#39:

As discussed on Slack, these docs should probably not be in the HIG section.

Suggestions for the reviewer doc:

  • Some info about rights management - otherwise a new reviewer may be confused if they cannot do certain things.
  • Try to avoid discouraging contributors if it necessary to turn down a PR. Maybe there are parts of it that could be submitted as separate PR(s).
  • Try to avoid discouraging contributors by starting a review and then not following up when requested changes are made. (The corollary of this is that contributors need to remember to re-request a review after addressing issues).

Suggestions for the contributor doc:

  • Some info about CI and what to do about failures
  • Something about conflicts arising with master and the need to keep PRs up to date and buildable while they are open for review.

@danirabbit
Copy link
Member

I'm inclined to merge and then we can follow up with more edits and info. This is a good start :)

@danirabbit danirabbit merged commit 307e1c5 into main May 13, 2021
@danirabbit danirabbit deleted the code-reviews branch May 13, 2021 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants