validate bpo number by levlaz · Pull Request #72 · python/bedevere

levlaz · 2017-11-09T06:46:07Z

Add helper function to check if the issue number exists on bpo.

set FAILURE_STATUS in the event that the issue does not exist.

Add helper function to check if the issue number exists on bpo. set FAILURE_STATUS in the event that the issue does not exist. Fix #15

codecov · 2017-11-09T06:48:33Z

Codecov Report

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

@@          Coverage Diff          @@
##           master    #72   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines         923    953   +30     
  Branches       55     56    +1     
=====================================
+ Hits          923    953   +30

Impacted Files	Coverage Δ
tests/test_bpo.py	`100% <100%> (ø)`	⬆️
bedevere/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 81f8490...154e963. Read the comment docs.

levlaz · 2017-11-09T06:50:20Z

 STATUS_CONTEXT = "bedevere/issue-number"
 # Try to keep descriptions at or below 50 characters, else GitHub's CSS will truncate it.
-_FAILURE_DESCRIPTION = 'No issue # in title or "skip issue" label found'
+_FAILURE_DESCRIPTION = 'Issue does not exit, No issue # in title or "skip issue" label found'


This felt a little dirty, but I was not sure if we wanted to "yet another" edge case of failure description. Happy to refactor this if we want to be more specific of the reason for failure.

Hmm, this might be too long as a status check message.
I'm thinking it can be a separate status check, maybe "bedevere/valid-issue"? @brettcannon, what do you think?

I don't think we need another status check (we are already at 4). Is there any reason the message can't be dynamically specified like for the NEWS.d check? In this instance the current message is the one that shows up if anything is found, but if an issue number is discovered but found not to exist, a different message saying something like "issue #NNNN not found at bugs.python.org" or something would be set while still flagging a status check failure.

Make sense?

Using a different message sounds good :)

Corrections to message: /exit/exist/, /No/no/, /title/title,/. But +1 to situation specific message. This is not an exception raised in inner loops for control flow. (The reason for some quick and vague exception messages.)

levlaz · 2017-11-09T06:51:04Z

    assert gh.patch_url is None
+
+
+def test_validate_issue_number():


This is a "unit" test but it actually makes a call to bpo... maybe this should be mocked as well (or it may not be necessary all together).

My preference is to mock the call to b.p.o.

Mariatta

Thanks for the PR! I have a few comments, mostly just nitpicky :)
I think the new failure description is a little too long. Perhaps it should be a separate status check?

Mariatta · 2017-11-10T03:35:48Z

 STATUS_CONTEXT = "bedevere/issue-number"
 # Try to keep descriptions at or below 50 characters, else GitHub's CSS will truncate it.
-_FAILURE_DESCRIPTION = 'No issue # in title or "skip issue" label found'
+_FAILURE_DESCRIPTION = 'Issue does not exit, No issue # in title or "skip issue" label found'


Hmm, this might be too long as a status check message.
I'm thinking it can be a separate status check, maybe "bedevere/valid-issue"? @brettcannon, what do you think?

Mariatta · 2017-11-10T03:36:49Z

                              target_url=url)
+
+def validate_issue_number(issue_number):
+    """Validate that the issue number exists on bpo"""


The docstring should end with a period :)

Mariatta · 2017-11-10T03:37:08Z

+        urllib.request.urlopen(req)
+        return True
+    except urllib.error.HTTPError:
+        return False


Needs a newline.

Mariatta · 2017-11-10T03:45:02Z

    assert gh.patch_url is None
+
+
+def test_validate_issue_number():


My preference is to mock the call to b.p.o.

Mariatta · 2017-11-10T03:49:15Z

@@ -1,5 +1,7 @@
 """Check if a bugs.python.org issue number is specified in the pull request's title."""
 import re
+import urllib.request


How about from urllib import error, request instead?

levlaz · 2017-11-12T04:52:39Z

Thank you for the review @Mariatta, I made all of the requested changes except for defining a new status check message since you want to get some feedback from Brett.

Happy to implement whatever you all decide.

brettcannon

Thanks for the PR! Let's go with different status check messages based on why the check failed. Otherwise a very minor tweak to a function name.

brettcannon · 2017-11-15T18:14:58Z

                              description=f"Issue number {issue_number} found",
                              target_url=url)
+
+def validate_issue_number(issue_number):


Start with an underscore since this shouldn't be used outside of this module.

merwok · 2017-11-26T18:22:33Z

+    url = f"https://bugs.python.org/issue{issue_number}"
+    req = request.Request(url=url, method='HEAD')
+    try:
+        res = request.urlopen(req)


Shouldn’t this use an async request instead of dear old urllib?

Mariatta · 2018-07-16T03:04:32Z

@levlaz still working on this?

lysnikolaou · 2019-03-21T01:46:55Z

Since @levlaz is apparently no longer working on this, should this PR may be closed? @Mariatta?

mariatta-bot · 2019-03-21T01:48:35Z

Mariatta was mentioned, but she's out of open source from March 18th to May 9th, 2019 . Be aware she might not get to this until after PyCon US.
(I'm a bot)

brettcannon · 2019-03-21T19:59:55Z

Due to this stalling out, I'm closing the PR.

validate bpo number

c224b7b

Add helper function to check if the issue number exists on bpo. set FAILURE_STATUS in the event that the issue does not exist. Fix #15

the-knights-who-say-ni added the CLA signed label Nov 9, 2017

levlaz commented Nov 9, 2017

View reviewed changes

Mariatta reviewed Nov 10, 2017

View reviewed changes

Mariatta requested a review from brettcannon November 10, 2017 03:56

levlaz added 2 commits November 11, 2017 20:50

clean up imports and add new line

5512009

mock requests to bpo

154e963

brettcannon requested changes Nov 15, 2017

View reviewed changes

merwok reviewed Nov 26, 2017

View reviewed changes

brettcannon closed this Mar 21, 2019

lysnikolaou mentioned this pull request Mar 23, 2019

Validate bpo issue number #159

Merged

		assert gh.patch_url is None


		def test_validate_issue_number():

Conversation

levlaz commented Nov 9, 2017

Uh oh!

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

Uh oh!

Codecov Report

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!

Mariatta 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!

Choose a reason for hiding this comment

Uh oh!

levlaz commented Nov 12, 2017

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!

Mariatta commented Jul 16, 2018

Uh oh!

lysnikolaou commented Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariatta-bot commented Mar 21, 2019

Uh oh!

brettcannon commented Mar 21, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov Bot commented Nov 9, 2017 •

edited

Loading

lysnikolaou commented Mar 21, 2019 •

edited

Loading