Create separate threads on GitHub Discussions #5
Conversation
There was a problem hiding this comment.
Hi @SD-13, I think this looks great, thanks for working on it so quickly!
I left a few inline comments. Just a few general notes:
- The jam.dev functionality looks good, but doesn't show what happens when you click into a discussion. Could you show it please?
- The CI checks are failing, might be worth taking a look at them.
Other than that, it looks really good to me ;) Thanks a lot!
| Hi {{ username }}, | ||
|
|
||
| It looks like you're assigned to this PR, but haven't taken any action for at least 2 days: | ||
| The following PRs are blocked on your review: |
There was a problem hiding this comment.
I think you'll still want to keep the "Hi {{username}}" bit, otherwise this is a bit abrupt?
Let's maybe also changed "blocked" here to "currently blocked".
| {{ pr_list }} | ||
|
|
||
| Please review and unassign yourself from the pending PRs as soon as possible. |
There was a problem hiding this comment.
Please keep this line, thanks. Let's also change it to: "Please review and unassign yourself from the pending PRs as soon as possible, then mark this discussion thread as 'Done'."
src/github_services.py
Outdated
| repo_name: str, | ||
| discussion_category: str, | ||
| ) -> None: | ||
| """Delete all existing discussions""" |
There was a problem hiding this comment.
Delete all existing discussions in the given discussion category.
src/github_services.py
Outdated
| discussion_title: str, | ||
| discussion_body: str | ||
| ): | ||
| """Create new discussion with given title and body.""" |
There was a problem hiding this comment.
Create a new discussion with the given title and body in the given discussion category.
src/main.py
Outdated
| """Generates message using the template provided in | ||
| PENDING_REVIEW_NOTIFICATION_TEMPLATE.md. | ||
|
|
||
| Args: | ||
| username: str. Reviewer username. | ||
| pr_list: str. List of PRs not reviewed within the maximum waiting time. |
There was a problem hiding this comment.
It's a bit odd that this is a string. Is it actually a string or is it a list?
If the former, maybe more info in the docstring about how this string is actually formatted would be helpful.
There was a problem hiding this comment.
Now it is taking PullRequest objects as input and joining it to string inside the function.
src/main.py
Outdated
| reviewer_name, prs, org_name, repo_name, discussion_category, discussion_title) | ||
| for reviewer_name, pr_list in reviewer_to_assigned_prs.items(): | ||
| discussion_title = f"Pending Reviews: @{reviewer_name}" | ||
| discussion_body = generate_message('\n'.join(str(pr) for pr in pr_list), TEMPLATE_PATH) |
There was a problem hiding this comment.
Ah, the first arg here seems to be doing some joining. Maybe better to do that in the generate_message() function instead?
There was a problem hiding this comment.
Yes, now we are doing it inside the function.
|
Hey @seanlip, here is a jam(https://jam.dev/c/d202211c-e4a0-476b-ba59-4409bdb6d225) of the discussions, you can also check out them here https://github.com/SD-13/test-pending-review-notifier/discussions/categories/notify-reviewers2. Please review the changes when you get time. Thanks! |
|
@SD-13 We need to get the stale notifier working on oppia/oppia as well. I'm making updates to the CI workflows on oppia/oppia in oppia/oppia#21363 and I've included the update there -- see oppia/oppia@3f7e63b . Just FYI! The new workflow should start working when that PR is merged :) |
Fixes #4
I tested the changes in my repo
https://jam.dev/c/d9844463-3bd6-4c09-bc59-423dfed0b582