New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

validate bpo number #72

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@levlaz

levlaz commented Nov 9, 2017

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

validate bpo number
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

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 9, 2017

Codecov Report

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

Impacted file tree graph

@@          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.

codecov bot commented Nov 9, 2017

Codecov Report

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

Impacted file tree graph

@@          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.

@@ -21,7 +23,7 @@
SKIP_ISSUE_LABEL = util.skip_label("issue")
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 comment has been minimized.

@levlaz

levlaz Nov 9, 2017

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.

@levlaz

levlaz Nov 9, 2017

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.

This comment has been minimized.

@Mariatta

Mariatta Nov 10, 2017

Member

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

Mariatta Nov 10, 2017

Member

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?

This comment has been minimized.

@brettcannon

brettcannon Nov 15, 2017

Member

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?

@brettcannon

brettcannon Nov 15, 2017

Member

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?

This comment has been minimized.

@Mariatta

Mariatta Nov 15, 2017

Member

Using a different message sounds good :)

@Mariatta

Mariatta Nov 15, 2017

Member

Using a different message sounds good :)

This comment has been minimized.

@terryjreedy

terryjreedy Nov 15, 2017

Member

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.)

@terryjreedy

terryjreedy Nov 15, 2017

Member

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.)

Show outdated Hide outdated tests/test_bpo.py
@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?

@@ -21,7 +23,7 @@
SKIP_ISSUE_LABEL = util.skip_label("issue")
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 comment has been minimized.

@Mariatta

Mariatta Nov 10, 2017

Member

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

Mariatta Nov 10, 2017

Member

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?

Show outdated Hide outdated bedevere/bpo.py
Show outdated Hide outdated bedevere/bpo.py
Show outdated Hide outdated tests/test_bpo.py
Show outdated Hide outdated bedevere/bpo.py

@Mariatta Mariatta requested a review from brettcannon Nov 10, 2017

@levlaz

This comment has been minimized.

Show comment
Hide comment
@levlaz

levlaz Nov 12, 2017

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.

levlaz commented Nov 12, 2017

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.

@@ -89,3 +92,13 @@ def create_success_status(found_issue):
return util.create_status(STATUS_CONTEXT, util.StatusState.SUCCESS,
description=f"Issue number {issue_number} found",
target_url=url)
def validate_issue_number(issue_number):

This comment has been minimized.

@brettcannon

brettcannon Nov 15, 2017

Member

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

@brettcannon

brettcannon Nov 15, 2017

Member

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

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

This comment has been minimized.

@merwok

merwok Nov 26, 2017

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

@merwok

merwok Nov 26, 2017

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

@Mariatta

This comment has been minimized.

Show comment
Hide comment
@Mariatta

Mariatta Jul 16, 2018

Member

@levlaz still working on this?

Member

Mariatta commented Jul 16, 2018

@levlaz still working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment