Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Closes #5007 - Add PR guidelines to README. #5008

Merged
merged 2 commits into from
Aug 31, 2019

Conversation

liuche
Copy link
Contributor

@liuche liuche commented Aug 29, 2019

Adding some PR guidelines that we discussed to the README. This also links to the shared android PR guidelines docs.

This is definitely only a first step!

I'd also consider: linking from the description of in the Fenix repo (at the top, where there's lots of visibility) to either this new PR section of the README, or copying the guidelines there directly.

I'll also send an email out, once we decide to merge this.

Open questions:

  • Should we discuss that we prioritize our roadmap (code and review) as part of this change?
  • Are we going to timebox the amount of time we spend on non-sprint reviews?

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@liuche liuche requested a review from boek August 29, 2019 00:57
@liuche liuche requested a review from a team as a code owner August 29, 2019 00:57
@codecov-io
Copy link

codecov-io commented Aug 29, 2019

Codecov Report

Merging #5008 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #5008   +/-   ##
========================================
  Coverage      8.72%   8.72%           
  Complexity      192     192           
========================================
  Files           231     231           
  Lines          9137    9137           
  Branches       1326    1326           
========================================
  Hits            797     797           
  Misses         8287    8287           
  Partials         53      53

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 946be9e...d72f6a5. Read the comment docs.

Copy link
Contributor

@sblatz sblatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+

Can we also include a comment mentioning to look at help wanted tickets? Those are ones that we generally think are great for contributors to pick up 😄

README.md Outdated

We encourage you to participate in this open source project. We love Pull Requests, Bug Reports, ideas, (security) code reviews or any other kind of positive contribution.
* Every PR must have **exactly** one issue associated with it.
* Write a clear explanation of what your PR is doing when opening the request.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make a note here about also including self-comments on PR's when necessary rather than just a summary of the work? I find these to be more useful :)

README.md Outdated
* Every PR must have **exactly** one issue associated with it.
* Write a clear explanation of what your PR is doing when opening the request.
* Keep PRs small and to the point. For extra code-health changes, either file a separate issue, or make it a separate PR that can be easily reviewed.
* Use micro-commits - this makes it easier and faster to review.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Use micro-commits. This makes it easier and faster to review.

@NotWoods NotWoods added the pr:needs-changes PRs that need some changes/fixes before they can land label Aug 30, 2019
@liuche
Copy link
Contributor Author

liuche commented Aug 31, 2019

I skipped the help-wanted mention bc it's already listed in under "good first issues" as well as in the CONTRIBUTING.md file we link to.

I also added a part explaining why we might be slow handling PRs, which Jeff brought up.

Some other things we could consider are making this more step-by-step, but in light of landing stuff early and iterating, I'm going to merge this in. Open to more feedback or re-opening the issue.

@liuche liuche merged commit ac950f6 into mozilla-mobile:master Aug 31, 2019
@liuche liuche deleted the pr-guidelines branch August 31, 2019 00:38
@liuche liuche removed the pr:needs-changes PRs that need some changes/fixes before they can land label Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants