-
Notifications
You must be signed in to change notification settings - Fork 3
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
docs: add pr-triage.md #39
Open
thehowl
wants to merge
5
commits into
gnolang:main
Choose a base branch
from
thehowl:patch-1
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
# Reviewing PRs | ||
|
||
> Some guidelines and pointers on how to review PRs effectively. | ||
|
||
## A culture for reviewing | ||
|
||
A few tips to review effectively: | ||
|
||
- **Proposals > Discussions > Feelings** | ||
- Use this hierarchy to decide how to best make a PR review comment. | ||
- A proposal shares the work of "figuring it out" with the author. If using GitHub's ["suggestions" feature](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request), the changes can be applied immediately by the author. | ||
- **Don't take reviews personally,** and make reviews on the code rather than the author. | ||
- It's OK to ask for clarification when things are unclear. | ||
- Don't feel that you need to make a fully in-depth review and that you are stupid because you don't understand it at first glance. Giving the full context of the pull request is the responsibility of the author, together with writing documentation and tests. | ||
- If a PR lacks sufficient explanation of what has been changed, asking the author to explain themselves is perfectly acceptable. | ||
- Prefer reaching consensus over voting. | ||
- Adopting the "Proposals > Discussions > Feelings", try to reach consensus when in conflict over a code change. This means either trying to convince each other using objective metrics, or trying to find a middle ground solution that can satisfy both point of views. | ||
- Don't [bikeshed](https://bikeshed.com/). | ||
- Creating long-winded discussions on trivial topics is really common. Try to recognize when the change you're requesting relates to an objective impact or is purely aesthetical. | ||
- For code formatting and practices, prefer introducing linters and formatting rules separately rather than discussing them in a PR. | ||
- Reviews are a way to learn. | ||
- Don't be afraid to review components of code you don't have full knowledge about. It can help you get better knowledge of how that component works. | ||
- Learning what other people are doing and how they are doing it can give you new insights into solving problems. It can also help you build a better mental map of how the whole system works. | ||
- Prefer asking for asking an in-depth description, rather than scheduling a call. | ||
- Other reviewers might have your same doubts. It's good if the answers you're given are as such out for everyone to see. | ||
- Documentation and tests are also good places to put long-term information! | ||
- [Humanizing Reviews](https://www.processimpact.com/articles/humanizing_reviews.pdf) is a nice read. | ||
|
||
## Preparing the review meeting | ||
|
||
Weekly, we hold a ["GitHub review meeting"](https://github.com/gnolang/meetings/issues/18). | ||
The meeting serves as a synchronous space to unblock development of key Pull | ||
Requests, especially those which need consensus from the team to move forward | ||
or a "philosophical check-in" to ensure the PR is aligned with the intended | ||
direction of the project. (ie., assess whether the changes are OK with Jae or | ||
other stakeholders.) | ||
|
||
The agenda of the review meeting is prepared by adding a PR to the ["Review | ||
Meeting"](https://github.com/orgs/gnolang/projects/4/views/1) project, in the | ||
"Agenda" column. Anyone can add PRs to this agenda. Who is in charge of [leading | ||
the review meeting](./pr-triage.md) should proceed to prioritize the agenda: | ||
|
||
- PRs which need quick rounds of consensus from the core team can be tackled | ||
first; mostly if it is expected that any arising discussions will take less | ||
than 5 minutes. | ||
- PRs which need "longer" conversations should be tackled next; but prioritizing | ||
those that are felt as the most pressing and urgent issues, either because | ||
of a pending deadline or because they're blocking other, significant work. | ||
|
||
The point of the meeting is not necessarily to merge PRs, but rather to unblock | ||
further work. The meeting focuses more strictly on PRs to ensure that whatever | ||
we're talking about has already a real, practical code-implementation; for any | ||
general questions about whether an issue should be tackled or not, the Monday | ||
weekly sync is a better place. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
# Pull Request Triage | ||
|
||
> A process to organize the incoming reviews of PRs and create a system to | ||
> effectively manage, week after week, all our ongoing work and ensure a swift | ||
> feedback loop. | ||
|
||
## Background | ||
|
||
- We have a linearly-growing number of PRs. We are not doing enough to catch up on the work that is being done; as such, a lot of high-quality code gets stuck in review forever. | ||
- When code is stuck in review for long, many PRs are "dropped" by the author due to lack of interest or shift of focus. This is exasperated when looking at external contributions. | ||
- The work of reviewing PRs is not evenly distributed among the team | ||
- This is not so much an issue per se (some enjoy more the discussions, some more drilling on code), but the imbalance is very large and the onus of reviewing is felt more like a chore even by those who actively engage in it. | ||
|
||
--- | ||
|
||
## Objective | ||
|
||
- Lower the time required for the feedback loop to at most 1 week for the biggest PRs, and 3-4 days for the regular ones. | ||
- Steadily work through our backlog of open PRs | ||
- Reduce merge conflicts | ||
- Prepare to scale for an open source community | ||
|
||
## Process | ||
|
||
- Every week, 2 members of [gno tech staff](https://github.com/orgs/gnolang/teams/tech-staff) are assigned to do PR triage for that week. | ||
- Triage and first-review each incoming PR (= new ready PR, or moved from draft to ready): | ||
- Place it in the correct milestone | ||
- Make corrections for clarity in the title, description and label | ||
- **Make a first PR review**, as in-depth as you can but deferring where you lack the expertise. | ||
- If the PR lacks context, needs more explanation to understand what it's doing, lacks tests or documentation, **feel free to request it and defer reviewing to after that is done.** | ||
- The objective here is to ensure the PR doesn't have any major problems and can be handed off for finer-grained review by a codeowner. | ||
- Defer reviews on Draft PRs, unless the author has specifically requested an initial review (but be broad, in these reviews; no need to be specific if the code is not finalized). | ||
- For ready PRs, add any review requests which are not already requested by CODEOWNERS. | ||
- Place the PR in the correct column in the Pull Requests board. | ||
- _Prioritize community pull requests,_ because they are not paid they lose interest 10 times faster. | ||
- **Prioritize this part of the process over any software development for the duration of the triage week.** | ||
- Prepare the agenda for the review meeting | ||
- See the adjacent [Reviewing PRs](./pr-reviewing.md) document for some indications on how to best prepare and structure the review meeting. | ||
- Before the GitHub review meeting, work through the PRs you've discussed and viewed this week. | ||
- Add them to the "Agenda" column on GitHub and re-order them to make sure that the most important ones are at the top. | ||
- On the agenda, anyone can add items, but it is your responsibility to ensure to prioritize them. (If you're up to speed on most PRs, this can take as little as 15 minutes, before the call) | ||
- Make sure to know the context for the PRs in the review column, so that you can briefly introduce the changes via voice before kicking off its discussion. | ||
- Morgan and Nemanja are available as alternative leads if neither of the triagers is comfortable with leading it. However, preparing the agenda and being available to give context on the discussed PRs is still the responsibility of those on rotation. | ||
- Push development of existing PRs along | ||
- There are often important PRs which are blocked for weeks awaiting reviews or feedback from the existing reviewers. | ||
- Use the Core Team board to see the status on other PRs and ongoing work, coordinate with Nemanja on which ones to ask for status updates. | ||
- Use our main internal communication channels (Signal Gno-Tech-Staff; Slack #gno-core-tech) to publicly ping team members on what should be tackled. | ||
- If the team members are lagging behind on providing status update, please go ahead and ask status updates individually | ||
|
||
### Planning the rotation | ||
|
||
- Managers are in charge of planning the rotation. Two weeks before the end of each "full cycle", managers will post the rotation for the following N/2 weeks (the "full cycle", where N = number of team members). | ||
- At the time of writing, the number of weeks in a full cycle is likely to be 5/6. | ||
- Team members must mark their calendars or request a change in the rotation if they have other commitments in their assigned week. (Holidays, conferences, ...) | ||
- Prefer same timezones (communication is key) | ||
- Prefer knowledge sharing (mix tm2 <> gnovm, or strong pr culture <> weak pr culture) | ||
|
||
## Tools | ||
|
||
There are some existing tools you can use to oversee incoming PRs. | ||
|
||
- Setting up notifications (and making sure they are always enabled for participating conversations) | ||
- Some prefer the GitHub UI, some emails. GitHub has fine-grained settings you can [set up](https://github.com/settings/notifications) (Custom Routing is a useful feature if you use emails). | ||
- Enabling notifications can help you be up-to-speed with any replies being posted on your triage week. | ||
- GitHub PR/Issue search sorting | ||
- There are some good sorting filters when looking at the [pull requests tab on GitHub](https://github.com/gnolang/gno/pulls) | ||
- The default view is a good option to see newly created PRs. | ||
- You can use "Least recently updated" to see the most "stale" conversations on the repository. | ||
- But "recently updated" too can be effective as well, to see what PRs have been updated and can likely move forward. | ||
- You can use the `-is:draft` query to filter out draft PRs. | ||
- Make sure to communicate regularly with your co-triager. Suggestions: | ||
- Check out the above example for an example of a "workflow" with your co-triager. | ||
- If you need a hand figuring out the agenda for the review meeting, ping Morgan. | ||
|
||
Unfortunately, at the time of writing, no tool properly encapsulates a feed for | ||
the "incoming PRs" (= new ready PR, or moved from draft to ready). Eventually, | ||
we'll develop one and this section will be updated. | ||
|
||
### TODO: Stats | ||
|
||
These are ideas for useful stats which illustrate the problem right now; I know of no tool which would let me pick them up, but I'm trying to figure out to get them because it would also help us see the difference after implementing this process. | ||
|
||
Proposed tool for stats: https://github.com/change-metrics/monocle | ||
|
||
- (open PRs - draft - closed PRs over time) | ||
- weekly PRs done per team member (anonymized) | ||
- average/median time for first and second review | ||
- number of reviews received per PR (internal / external) | ||
- time-to-completion internal/external percentiles | ||
- feedback loop (time between author event (create / ready_for_review, commit, force-push) --> review/close) percentiles |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your text is good, but for anyone looking but I believe it's important to emphasize that the main goal is to shorten the feedback loop, not just the merging process. Shortening the feedback loop can speed up merging, but it's crucial to maintain quality and ensure reviews by the right code owners are mandatory, especially as time goes on.
Code owners, who may not be employees, are not required to strictly adhere to this employee convention.
We can implement an incentive model in the Review DAO to shorten the feedback loop by offering bonuses based on speed.