Automatically hyperlink `bpo-` text. (#103) #121

storymode7 · 2018-07-14T20:55:33Z

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)

codecov · 2018-07-14T20:57:30Z

Codecov Report

Merging #121 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #121    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          20     20            
  Lines        1332   1482   +150     
  Branches       72     85    +13     
======================================
+ Hits         1332   1482   +150

Impacted Files	Coverage Δ
tests/test_bpo.py	`100% <100%> (ø)`	⬆️
bedevere/bpo.py	`100% <100%> (ø)`	⬆️
bedevere/backport.py	`100% <0%> (ø)`	⬆️
tests/test_backport.py	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44aff09...1840cf4. Read the comment docs.

Mariatta

Thanks! Great start. I have some nitpicky comments.
I also think the hyperlinking should be done when the PR is opened/edited.

Mariatta · 2018-07-16T02:39:50Z

tests/test_bpo.py

+    data = {
+        "action": "created",
+        "comment": {
+            "url":"https://api.github.com/repos/blah/blah/issues/comments/123456",


Space after "url":.
Same for the lines 302, 319, 320, 336, 337, 358, 359

Mariatta · 2018-07-16T02:49:34Z

bedevere/bpo.py

@@ -83,10 +83,67 @@
        await set_status(event, gh)


+@router.register("issue_comment", action="edited")
+@router.register("issue_comment", action="created")


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"].

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.

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.

Okay. Thanks!
I get it now 🙂

Mariatta · 2018-07-16T02:52:36Z

bedevere/bpo.py

+                                   \s*bpo-(?P<issue>{issue})\s*
+                                   </a>""".format(issue=issue),
+                                   re.VERBOSE)
+    # The following approach takes care of cases like bpo-123 [bpo-123]...


Maybe the comments here and in line 126 should be part of docstring instead of inline codes.

Mariatta · 2018-07-16T02:53:00Z

bedevere/bpo.py

+def create_hyperlink_in_comment_body(body):
+    new_body = ''
+    leftover_body = body
+    # Using infinite while loop as it supports updation of the string being


I would prefer these comments as docstrings.

Mariatta

Overall looks great. Just some other nitpicky things..

Mariatta · 2018-07-21T01:44:41Z

bedevere/bpo.py

+
+
+def create_hyperlink_in_comment_body(body):
+    """Uses infinite loop for updating the string being searched dynamically"""


Missing period at the end of the sentence.

Mariatta · 2018-07-21T02:02:21Z

tests/test_bpo.py

+@pytest.mark.asyncio
+async def test_set_comment_body_success():
+    data = {
+        "action": "opened",


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.

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 🙂

Mariatta · 2018-07-21T02:04:24Z

tests/test_bpo.py

+    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


Please use double quotes to be consistent with the rest.

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 🙂

Mariatta · 2018-07-21T02:04:58Z

bedevere/bpo.py

+
+def check_hyperlink(match):
+    """The span checking of regex matches takes care of cases like bpo-123 [bpo-123]…"""
+    issue = match.group('issue')


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

Thanks! I removed a couple extra blank lines, but I think the rest looks great :)

Mariatta · 2018-07-27T04:09:32Z

Thanks!! 🌮

serhiy-storchaka · 2018-10-10T06:57:26Z

bedevere/bpo.py

+        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>)")


See python/core-workflow#292. I think www. is redundant here.

Yes, that's not accurate.

serhiy-storchaka · 2018-10-10T06:58:54Z

bedevere/bpo.py

+    """Uses infinite loop for updating the string being searched dynamically."""
+    new_body = ""
+    leftover_body = body
+    ISSUE_RE = re.compile(r"bpo-(?P<issue>\d+)")


Aren't word boundary anchors \b needed here?

Probably wouldn't hurt.

Automatically hyperlink bpo- text. (python#103)

4d98e95

the-knights-who-say-ni added the CLA signed label Jul 14, 2018

Mariatta requested changes Jul 16, 2018

View reviewed changes

storymode7 force-pushed the hyperlink-bpo-text branch from 8810c56 to 436bdd4 Compare July 18, 2018 11:35

Add pull_request event

3c53aba

storymode7 force-pushed the hyperlink-bpo-text branch from 436bdd4 to 3c53aba Compare July 18, 2018 11:58

Mariatta requested changes Jul 21, 2018

View reviewed changes

storymode7 and others added 2 commits July 25, 2018 00:15

Make quotes consistent and add more test cases

5781017

remove extra blank lines

1840cf4

Mariatta approved these changes Jul 25, 2018

View reviewed changes

Mariatta merged commit 93a67e3 into python:master Jul 27, 2018

serhiy-storchaka reviewed Oct 10, 2018

View reviewed changes

tirkarthi mentioned this pull request Oct 17, 2018

Automatically hyperlinking of bpo- text sometimes causes confusion #136

Closed

storymode7 deleted the hyperlink-bpo-text branch October 21, 2018 19:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically hyperlink `bpo-` text. (#103) #121

Automatically hyperlink `bpo-` text. (#103) #121

storymode7 commented Jul 14, 2018 •

edited

Loading

codecov bot commented Jul 14, 2018 •

edited

Loading

Mariatta left a comment

Mariatta Jul 16, 2018

Mariatta Jul 16, 2018

storymode7 Jul 16, 2018 •

edited

Loading

Mariatta Jul 17, 2018

storymode7 Jul 17, 2018

Mariatta Jul 16, 2018

Mariatta Jul 16, 2018

Mariatta left a comment

Mariatta Jul 21, 2018

Mariatta Jul 21, 2018

storymode7 Jul 24, 2018

Mariatta Jul 21, 2018

storymode7 Jul 24, 2018

Mariatta Jul 21, 2018

Mariatta left a comment

Mariatta commented Jul 27, 2018

serhiy-storchaka Oct 10, 2018

brettcannon Oct 10, 2018

serhiy-storchaka Oct 10, 2018

brettcannon Oct 10, 2018



		def create_hyperlink_in_comment_body(body):
		"""Uses infinite loop for updating the string being searched dynamically"""

Automatically hyperlink bpo- text. (#103) #121

Automatically hyperlink bpo- text. (#103) #121

Conversation

storymode7 commented Jul 14, 2018 • edited Loading

codecov bot commented Jul 14, 2018 • edited Loading

Codecov Report

Mariatta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

storymode7 Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mariatta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mariatta left a comment

Choose a reason for hiding this comment

Mariatta commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Automatically hyperlink `bpo-` text. (#103) #121

Automatically hyperlink `bpo-` text. (#103) #121

storymode7 commented Jul 14, 2018 •

edited

Loading

codecov bot commented Jul 14, 2018 •

edited

Loading

storymode7 Jul 16, 2018 •

edited

Loading