Add a link to b.p.o as a comment by kushaldas · Pull Request #24 · python/bedevere

kushaldas · 2017-06-26T04:57:45Z

Adds the comment in the body of the PR.

codecov · 2017-06-26T05:00:16Z

Codecov Report

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

@@          Coverage Diff          @@
##           master    #24   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         289    319   +30     
  Branches       11     13    +2     
=====================================
+ Hits          289    319   +30

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

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 f4f4f7e...1f2fffd. Read the comment docs.

brettcannon

Only like two things that should probably change, and the rest is just suggestions from me to potentially simplify things (let me know what you think).

brettcannon · 2017-06-26T22:44:07Z

+re3='((?:http|https)(?::\\/{2}[\\w]+)(?:[\\/|\\.]?)(?:[^\\s"]*))'	# HTTP URL 1
+re4='.*?'	# Non-greedy match on filler
+re5='(<!--.*?-->)'	# HTML Comment 2
+BPO_URL_RE = re.compile(re1+re2+re3+re4+re5,re.IGNORECASE|re.DOTALL)


If you use re.VERBOSE you can do it as a multi-line string and not have to do the concatenation (https://docs.python.org/3/library/re.html#re.X). (But I have a suggestion below which if it makes sense means you don't need the regex 😃 ).

brettcannon · 2017-06-26T22:45:27Z

        else:
            status = FAILURE_STATUS
    else:
+        if "body" in event.data["pull_request"]:


Is "body" ever not in the pull request?

Yes, there are edits (iirc) which does not have body, my test instance crashed due to this.

brettcannon · 2017-06-26T22:59:11Z

    else:
+        if "body" in event.data["pull_request"]:
+            body = event.data["pull_request"]["body"]
+            bpo_url_found = BPO_URL_RE.search(body)


I think I might know of a way to simplify this. If you made the format of the message be:

TAG_NAME = "issue-number CLOSING_TAG = f"" BPO_MSG = f"""{{body}}  {{message}} {CLOSING_TAG} """

Then you can do:

if CLOSING_TAG not in body: ...

That way the search is a straight-forward string search and you leave the re module out of it. And by embedding the issue number in the comment you add a little bit of extra detail that may be useful later (e.g. if someone adds support to detect a change of issue number in the title and wants to update the comment accordingly but doesn't want to worry about any format change in the message itself).

This is something similar I did at first (personally I don't use re module that much). I will use this msg text now.

brettcannon · 2017-06-26T22:59:40Z

+            body = event.data["pull_request"]["body"]
+            bpo_url_found = BPO_URL_RE.search(body)
+            if not bpo_url_found:
+                issue_number = issue_number_found.group(1)


issue_number_found.group("issue")

brettcannon · 2017-06-26T23:04:37Z

+BPO_URL_RE = re.compile(re1+re2+re3+re4+re5,re.IGNORECASE|re.DOTALL)
+BPO_MSG = """{0}
+<!-- issue number -->
+{1}


Would it make sense to have a fancier message? E.g. "(Please only discuss the code in this pull request here on GitHub; all other discussion should occur on the issue at bpo-{issue_number}.)"

brettcannon · 2017-06-26T23:05:44Z

+            bpo_url_found = BPO_URL_RE.search(body)
+            if not bpo_url_found:
+                issue_number = issue_number_found.group(1)
+                bpo_url = f"https://bugs.python.org/issue{issue_number}"


You could add this to BPO_MSG so that it's all part of the message template.

brettcannon

Since the changes I'm requesting are minor, I'm approving this now so you can merge once you make the changes!

brettcannon · 2017-06-30T00:00:54Z

+    gh = FakeGH()
+    await bpo.router.dispatch(event, gh)
+    assert gh.patchdata is None
+    assert gh.patchurl is None


Missing a newline at the end of the file.

brettcannon · 2017-06-30T00:01:00Z

+            if CLOSING_TAG not in body:
+                issue_number = issue_number_found.group("issue")
+                BPO_MSG = f"""{body}\n
+<!-- {TAG_NAME}: bpo-{issue_number} -->


To make this more readable, you can indent it and then use textwrap.dedent() to strip the leading whitespace.

brettcannon · 2017-06-30T00:10:44Z

+            body = event.data["pull_request"]["body"]
+            if CLOSING_TAG not in body:
+                issue_number = issue_number_found.group("issue")
+                BPO_MSG = f"""{body}\n


Might as well just have an actual newline in the triple-quoted string rather than a newline literal (I actually overlooked it when I was checking that there was a newline separating the body from the link).

brettcannon · 2017-06-30T00:12:17Z


    def __init__(self, *, getitem=None):
        self._getitem_return = getitem
+        self.patchurl = None


Underscores have been used in the rest of the test code, so patch_url and patch_data for consistency.

brettcannon · 2017-07-14T18:15:16Z

I just wanted to check in and make sure you're not blocked on me (no pressure, I just don't want to be a hold-up for you).

brettcannon · 2017-07-14T18:57:07Z

I also had the time to address my own comments, so if you're happy with the resulting patch let me know and I will merge it!

brettcannon · 2017-07-17T21:49:39Z

One thing I've noticed while playing with other GitHub stuff is they use \r\n for line separators in the body. Is that something we need to worry about when re-constructing the body?

kushaldas · 2017-07-18T14:54:34Z

The examples I can see don't have \r\n
Like

"body": "Can we merge this without testing?\n<!-- issue-number: bpo-30442 -->\nhttps://bugs.python.org/issue30442\n<!-- /issue-number -->\n",

kushaldas · 2017-07-18T14:55:29Z

The commits look good, you can go ahead and merge. I got stuck in something else for the last 2 weeks :(

berkerpeksag · 2017-07-21T22:22:26Z

+            body = event.data["pull_request"]["body"]
+            if CLOSING_TAG not in body:
+                issue_number = issue_number_found.group("issue")
+                BPO_MSG = textwrap.dedent(f"""\


Unfortunately, textwrap.dedent() doesn't work with f-strings (or there is some other reason I don't know) See https://github.com/python/bedevere/pull/27/files/dcc17d64f1c9a9d6b65703abbf697c132d76d785#r127503862 for the previous discussion.

You can see the example in my latest PR: python/cpython#2807

Already have a fix waiting for travis to clear.

And it's merged, so once Travis is green for master again it will go live.

And I don't know why textwrap.dedent() isn't working since I don't see how an f-string would influence the outcome in anyway. My guess is the string as being inserted has some unexpected whitespace that's throwing the algorithm off.

Is this a bug with textwrap.dedent() or f-strings?
Worth opening an issue in b.p.o about this?

Nope, I see what happened. Berker's body had newlines in it that led to parts of the body not being equally indented like the string literal, so when textwrap.dedent() got it there was whole chunks of text not indented so it assumed there was nothing to dedent. IOW it worked as intended and it just didn't occur to me when doing the code review that the inserted body would have newlines followed by text that wouldn't be indented.

Fixes python#3 adds the link to b.p.o as a comment

cb5c042

the-knights-who-say-ni added the CLA signed label Jun 26, 2017

brettcannon changed the title ~~Fixes #3 adds the link to b.p.o as a comment~~ Add a link to b.p.o as a comment Jun 26, 2017

brettcannon requested changes Jun 26, 2017

View reviewed changes

Fixes python#3: Uses simple string to find the issue-number

3a0702e

brettcannon approved these changes Jun 30, 2017

View reviewed changes

brettcannon added 3 commits July 14, 2017 11:41

Add a newline at the end of the file

9c691cc

Use textwrap.dedent() to make the new message body easier to read

0b71a91

Tweak some attributes names

3dcf4a1

Inline a function that is only used once

1f2fffd

brettcannon merged commit 3de06e3 into python:master Jul 21, 2017

berkerpeksag reviewed Jul 21, 2017

View reviewed changes

Uh oh!

Uh oh!

Conversation

kushaldas commented Jun 26, 2017

Uh oh!

codecov Bot commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brettcannon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Jul 14, 2017

Uh oh!

brettcannon commented Jul 14, 2017

Uh oh!

brettcannon commented Jul 17, 2017

Uh oh!

kushaldas commented Jul 18, 2017

Uh oh!

kushaldas commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Jun 26, 2017 •

edited

Loading

brettcannon left a comment •

edited

Loading