Skip to content

docs: formalized '1 approval' and 'team assigned' labels #9861

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

Merged
merged 6 commits into from
Aug 30, 2024
Merged

docs: formalized '1 approval' and 'team assigned' labels #9861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dccfa23
docs: formalized '1 approval' and 'team assigned' labels
JoshuaKGoldberg Aug 22, 2024
aa7f2c8
Update docs/maintenance/Pull_Requests.mdx
JoshuaKGoldberg Aug 22, 2024
3abac14
Mention time critical tasks
JoshuaKGoldberg Aug 23, 2024
3d8a2f5
Update docs/maintenance/Pull_Requests.mdx
JoshuaKGoldberg Aug 23, 2024
cf6da79
remove unnecessary tip
JoshuaKGoldberg Aug 26, 2024
04e532e
Added note on waiting periods
JoshuaKGoldberg Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/pr-review-requested.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ jobs:
steps:
- uses: actions-ecosystem/action-remove-labels@v1
with:
labels: 'awaiting response'
labels: |
1 approval
awaiting response
- if: failure()
run: |
echo "Don't worry if the previous step failed."
Expand Down
6 changes: 4 additions & 2 deletions docs/maintenance/Issues.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ Most issues go through the following review flow when created or updated:
- Close the issue as _not planned_
- If the issue requires further discussion or community engagement evaluation:
- Add the `evaluating community engagement` label and remove the `triage` label
2. If the report is valid, add the `accepting prs` label and remove the `triage` label
2. If the report is valid:
- Remove the `triage` label
- Add the `team assigned` label if you think only a member of the team should tackle it, or `accepting prs` if anybody could
3. If you know the rough steps for a fix, consider writing a comment with links to codebase to help someone put together a fix
4. If you think that the fix is relatively straightforward, consider also adding the `good first issue` label

Expand Down Expand Up @@ -97,7 +99,7 @@ However, there are cases when maintainers can't confidently choose the most reas

3-6 months after the issue is labeled `evaluating community engagement`, the engagement of community is evaluated:

- If the community was active and common ground was found, the issue is labeled as `accepting prs`
- If the community was active and common ground was found, the issue is labeled as `accepting prs` or `team assigned`
- Otherwise, the issue is closed as _not planned_ with the `wontfix` label

For requests that can be implemented outside of typescript-eslint, be sure to mention any relevant APIs such as [Custom Rules](../developers/Custom_Rules.mdx) that can be used.
Expand Down
20 changes: 16 additions & 4 deletions docs/maintenance/Pull_Requests.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Open pull requests ideally switch between two states:
- Ready for review: either newly created or after [review is re-requested](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews#re-requesting-a-review)
- Waiting for author activity: either by virtue of [being a draft](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests#draft-pull-requests) or having the [`awaiting response` label](https://github.com/typescript-eslint/typescript-eslint/pulls?q=is%3Apr+is%3Aopen+label%3A%22awaiting+response%22)

Add the `awaiting response` label to a PR whenever you post a review.
Add the `awaiting response` label to a PR and remove `1 approval` if it exists whenever you post a request for changes.
It will be automatically removed if the author re-requests review.

### Be Kind
Expand Down Expand Up @@ -66,9 +66,6 @@ If there's no backing issue:
- If the PR is a straightforward docs or tooling fix that doesn't impact users, it's ok to review it as-is
- Otherwise, add the `please fill out the template` label and post a comment like the _Needs Issue_ template

Upon completing your review, if the build is passing and you feel comfortable merging it in, go ahead and squash merge.
Otherwise, add the `1 approval` label.

#### Common Things to Look For

- Style: that can you read through the code easily, there are comments for necessary tricky areas, and not unnecessary comments otherwise.
Expand Down Expand Up @@ -111,6 +108,21 @@ For preliminary reviews, be clear with what scale you're reviewing at: _"Reviewi
For repeat reviews, be clear when it's something you missed earlier and/or there's new information.
Don't apologize if the missed information was only made clear because of requested changes - this is part of the review process!

### Approvals

If the PR addresses a time critical task, such as a security fix or `main` branch build break, go ahead and squash merge it.

Otherwise, upon completing your review, if the build is passing and you have no blockers, approve it on GitHub.
Then:

- If there isn't a `1 approval` label or existing approval, add the `1 approval` label
- If there's already `1 approval` and/or it's been a week since the last request for changes, go ahead and squash merge
- For straightforward PRs that don't impact users, you can wait 3 days instead

There's no need to reset waiting periods for minor fixups from code reviews of otherwise approved code.

We try to leave PRs open for at least a week to give reviewers who aren't active every day a chance to get to it.
Copy link
Member Author

Choose a reason for hiding this comment

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

Made this a bit more clear/precise: it's a week in review, not a week since approval. IMO no need to wait a week after approval for something that's already been open for a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good call on clarifying that. That's a better approach than what my understanding was! 👍


### Other States

#### External Blockers
Expand Down
Loading