Skip to content

Commit

Permalink
Rename needs_work to awaiting_changes
Browse files Browse the repository at this point in the history
Matching the new set of statuses in
#29.
  • Loading branch information
timokau committed Jun 24, 2020
1 parent 063ec63 commit 13d16cf
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 9 deletions.
4 changes: 2 additions & 2 deletions USAGE.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
This bot allows everybody to change a PRs status with a simple comment of the form `/status <new_status_here>`. The right status of a PR depends on which group of people it is currently *actionable* to. You can set the state to

- `needs_review`, if the PR author considers this PR ready and needs a peer review. Issues of this state should actionable to *anybody* who is willing to help out and spend some time on PR reviews. No special privileges or experience necessary.
- `needs_work` if the PR in its current form is not ready yet. Issues of this state are usually only actionable to the PR author, although outside help is of course also possible.
- `awaiting_changes` if the PR in its current form is not ready yet. Issues of this state are usually only actionable to the PR author, although outside help is of course also possible.
- `needs_merge` can be set by reviewers who do not have merge permission but *would merge this PR if they could*. Think of this as a merge-button by proxy. PRs of this state are actionable for contributors with merge permission. These contributors may have further feedback, but the reviewer should make an honest effort to anticipate the feedback and get all issues resolved *before* setting the state to `needs_merge`.

# Tips for Reviewers
Expand All @@ -12,4 +12,4 @@ As explained in the previous section, any PR with the `needs_review` label shoul

- If you don't understand parts of the changes: Give a review and ask the PR author for clarifications! The PR author should then usually add the clarifications as comments to the nix expression. This is very valuable feedback. In fact, sometimes it is useful to be a bit naive on purpose. All source code should be well-documented and commented.

- If you think somebody with a very specific expertise should look at the PR: Try to find out who that could be (for example by looking at similar files and who has changed them in the past) and ping them. Delegation is also an action, and often the best one. If you have done so, set the state back to `needs_work` since the PR is no longer actionable to any reviewer. If the person you pinged is unresponsive, the PR author can set the state back to `needs_review` after some reasonable amount of time (usually something in the order of 3 days to a week).
- If you think somebody with a very specific expertise should look at the PR: Try to find out who that could be (for example by looking at similar files and who has changed them in the past) and ping them. Delegation is also an action, and often the best one. If you have done so, set the state back to `awaiting_changes` since the PR is no longer actionable to any reviewer. If the person you pinged is unresponsive, the PR author can set the state back to `needs_review` after some reasonable amount of time (usually something in the order of 3 days to a week).
10 changes: 6 additions & 4 deletions marvin/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
BOT_NAME = os.environ.get("BOT_NAME", "marvin-mk2")

# List of mutually exclusive status labels
ISSUE_STATUS_LABELS = {"needs_review", "needs_work", "needs_merge"}
ISSUE_STATUS_LABELS = {"needs_review", "awaiting_changes", "needs_merge"}

GREETING = f"""
Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage [here](https://github.com/timokau/marvin-mk2/blob/deployed/USAGE.md).
Expand Down Expand Up @@ -113,8 +113,8 @@ async def handle_comment(
# Only handle one command for now, since a command can modify the issue and
# we'd need to keep track of that.
for command in commands:
if command == "status needs_work":
await set_issue_status(issue, "needs_work", gh, token)
if command == "status awaiting_changes":
await set_issue_status(issue, "awaiting_changes", gh, token)
elif command == "status needs_review":
await set_issue_status(issue, "needs_review", gh, token)
elif command == "status needs_merge":
Expand Down Expand Up @@ -175,7 +175,9 @@ async def pull_request_review_submitted_event(
event: sansio.Event, gh: GitHubAPI, token: str, *args: Any, **kwargs: Any
) -> None:
if event.data["review"]["state"] == "changes_requested":
await set_issue_status(event.data["pull_request"], "needs_work", gh, token)
await set_issue_status(
event.data["pull_request"], "awaiting_changes", gh, token
)
await handle_comment(
event.data["review"],
event.data["pull_request"],
Expand Down
6 changes: 3 additions & 3 deletions tests/test___main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ async def test_removes_old_status_labels_on_new_status() -> None:
"user": {"id": 42, "login": "somebody"},
"labels": [
{"name": "marvin"},
{"name": "needs_work"},
{"name": "awaiting_changes"},
{"name": "needs_merge"},
],
},
Expand All @@ -64,7 +64,7 @@ async def test_removes_old_status_labels_on_new_status() -> None:
assert gh.post_data == [("issue-url/labels", {"labels": ["needs_review"]})]
assert set(gh.delete_urls) == {
"issue-url/labels/needs_merge",
"issue-url/labels/needs_work",
"issue-url/labels/awaiting_changes",
}


Expand All @@ -86,7 +86,7 @@ async def test_responds_to_pull_request_summary_commands() -> None:
gh = GitHubAPIMock()
await main.router.dispatch(event, gh, token="fake-token")
assert gh.post_data == [
("pr-url/labels", {"labels": ["needs_work"]}),
("pr-url/labels", {"labels": ["awaiting_changes"]}),
("pr-url/labels", {"labels": ["needs_review"]}),
]
assert set(gh.delete_urls) == {
Expand Down

0 comments on commit 13d16cf

Please sign in to comment.