Automatically hyperlink bpo- text. (#103)#121
Automatically hyperlink bpo- text. (#103)#121Mariatta merged 4 commits intopython:masterpython/bedevere:masterfrom storymode7:hyperlink-bpo-textstorymode7/bedevere:hyperlink-bpo-textCopy head branch name to clipboard
bpo- text. (#103)#121Conversation
Codecov Report
@@ Coverage Diff @@
## master #121 +/- ##
======================================
Coverage 100% 100%
======================================
Files 20 20
Lines 1332 1482 +150
Branches 72 85 +13
======================================
+ Hits 1332 1482 +150
Continue to review full report at Codecov.
|
Mariatta
left a comment
There was a problem hiding this comment.
Thanks! Great start. I have some nitpicky comments.
I also think the hyperlinking should be done when the PR is opened/edited.
| data = { | ||
| "action": "created", | ||
| "comment": { | ||
| "url":"https://api.github.com/repos/blah/blah/issues/comments/123456", |
There was a problem hiding this comment.
Space after "url":.
Same for the lines 302, 319, 320, 336, 337, 358, 359
|
|
||
|
|
||
| @router.register("issue_comment", action="edited") | ||
| @router.register("issue_comment", action="created") |
There was a problem hiding this comment.
I'm thinking that the hyperlinking should also be done on pull_request opened and edited events.
The body will be event.data["pull_request"]["body"] instead of event.data["comment"]["body"].
And the patch url to be used in line 93 will be event.data["pull_request"]["issue_url"].
There was a problem hiding this comment.
Thanks for reviewing! 🙂
Just a small question:
Wouldn't it be better if the bot hyperlinks bpo- text in the comment? As
- the pull request body already contains the link of the issue.
- Also, few (510 out of 7,525) PRs have
bpo-text in the body.
Please correct me if I am wrong.
There was a problem hiding this comment.
Yeah I meant to do this in both issue_comment and pull_request body.
The PR body already contains the link to the bpo that is mentioned in the PR title, and we should leave that as is.
I just think it would be nice to be also able to refer to other bpo tickets in the PR body.
There was a problem hiding this comment.
Okay. Thanks!
I get it now 🙂
| \s*bpo-(?P<issue>{issue})\s* | ||
| </a>""".format(issue=issue), | ||
| re.VERBOSE) | ||
| # The following approach takes care of cases like bpo-123 [bpo-123]... |
There was a problem hiding this comment.
Maybe the comments here and in line 126 should be part of docstring instead of inline codes.
| def create_hyperlink_in_comment_body(body): | ||
| new_body = '' | ||
| leftover_body = body | ||
| # Using infinite while loop as it supports updation of the string being |
There was a problem hiding this comment.
I would prefer these comments as docstrings.
8810c56 to
436bdd4
Compare
436bdd4 to
3c53aba
Compare
Mariatta
left a comment
There was a problem hiding this comment.
Overall looks great. Just some other nitpicky things..
|
|
||
|
|
||
| def create_hyperlink_in_comment_body(body): | ||
| """Uses infinite loop for updating the string being searched dynamically""" |
There was a problem hiding this comment.
Missing period at the end of the sentence.
| @pytest.mark.asyncio | ||
| async def test_set_comment_body_success(): | ||
| data = { | ||
| "action": "opened", |
There was a problem hiding this comment.
You're only testing for the pull_request opened event.
Can you also test for the pull_request edited, issue_comment created, and issue_comment edited events?
Probably you can use pytest.mark.parametrize for it.
And please do the same for the other two tests below.
There was a problem hiding this comment.
Thanks! pytest.mark.parametrize was great!
I've used the changes key in the pull_request event's data for both action opened and edited. It keeps the both the cases to be used from single function. I hope it wont be a problem 🙂
| await bpo.router.dispatch(event, gh) | ||
| body_data = gh.patch_data | ||
| assert body_data["body"].count("[bpo-123](https://www.bugs.python.org/issue123)") == 2 | ||
| assert '123456' in gh.patch_url |
There was a problem hiding this comment.
Please use double quotes to be consistent with the rest.
There was a problem hiding this comment.
Really sorry for this error. I fixed this, and there was one previous occurence, so I fixed that too.
If the change for previous occurence of single quotes shouldn't have been done, I'll be happy to revert that change 🙂
|
|
||
| def check_hyperlink(match): | ||
| """The span checking of regex matches takes care of cases like bpo-123 [bpo-123]…""" | ||
| issue = match.group('issue') |
There was a problem hiding this comment.
Please use double quotes for your changes.
I know we haven't been consistent in the past, but since this is new code, might as well be more consistent about it. :)
Mariatta
left a comment
There was a problem hiding this comment.
Thanks! I removed a couple extra blank lines, but I think the rest looks great :)
|
Thanks!! 🌮 |
| if presence is False: | ||
| new_body = new_body + leftover_body[:match.start()] | ||
| leftover_body = leftover_body[match.end():] | ||
| new_body = new_body + match.expand("[bpo-\g<issue>](https://www.bugs.python.org/issue\g<issue>)") |
There was a problem hiding this comment.
See python/core-workflow#292. I think www. is redundant here.
| """Uses infinite loop for updating the string being searched dynamically.""" | ||
| new_body = "" | ||
| leftover_body = body | ||
| ISSUE_RE = re.compile(r"bpo-(?P<issue>\d+)") |
There was a problem hiding this comment.
Aren't word boundary anchors \b needed here?
Hey, This aims to fix #103 .
If anything needs any improvement please hint me, I'll fix it up in the best manner I can 😄
(I added the changes to bpo.py, if you find it inappropriate, please suggest where it'd suite better)