Skip to content
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

Accept GitHub issues numbered only 32426 or above #519

Merged
merged 6 commits into from
Dec 29, 2023

Conversation

menkotoglou
Copy link
Contributor

Fixes #504.

Iterates through the template file to find the gh-issue line and checks number if it's in the accepted issue number range.

This is how an unsuccessful attempt looks like:

❯ /bin/python3 /path/core-workflow/blurb/blurb.py

Error: The gh-issue number should be 32426 or above.

[Hit return to retry (or Ctrl-C to abort)>

Copy link

cpython-cla-bot bot commented Dec 17, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

blurb/blurb.py Outdated Show resolved Hide resolved
blurb/blurb.py Outdated Show resolved Hide resolved
blurb/blurb.py Outdated Show resolved Hide resolved
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please could you add some tests?

Hopefully we can get #520 merged soon, then we can add unit tests.

In the meantime, tests are done by adding valid blurb files in the pass dir and invalid ones in the fail dir at https://github.com/python/core-workflow/tree/main/blurb/tests, and then running blurb test.

blurb/blurb.py Outdated Show resolved Hide resolved
@menkotoglou
Copy link
Contributor Author

Seems like the not_number case is already covered in the code. That's validated in the pre-existing invalid-gh-number.rst test case.

Also, the tests wouldn't pass if we didn't raise an exception at the class Blurbs level. Decided to move to the class definition level as I'm aligned with this approach.

Finally, retrospectively fixed some old tests that indeed were using smaller GH issue numbers.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It's nice when adding tests reveals a problem in the original implementation :)

Looks good, here's a wording suggestion.

blurb/blurb.py Outdated Show resolved Hide resolved
@menkotoglou
Copy link
Contributor Author

Thanks a lot for the review @hugovk. Suggestion commited!

@menkotoglou menkotoglou requested a review from hugovk December 20, 2023 12:11
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of non-blocking suggestions, with one of them being a nitpick that you're welcome to ignore if it's too much effort and a request to name a magic number.

blurb/blurb.py Outdated Show resolved Hide resolved
blurb/blurb.py Outdated Show resolved Hide resolved
@menkotoglou
Copy link
Contributor Author

Thanks a lot for the review @pradyunsg. Feedback addressed already!

blurb/blurb.py Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <[email protected]>
blurb/blurb.py Outdated Show resolved Hide resolved
Co-authored-by: Pradyun Gedam <[email protected]>
@menkotoglou
Copy link
Contributor Author

Hey @pradyunsg, @hugovk, you think we're OK to merge this?

@hugovk
Copy link
Member

hugovk commented Dec 29, 2023

Yes, let's merge. Thank you for your contribution!

@hugovk hugovk merged commit c9c3e4a into python:main Dec 29, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blurb: validate GitHub issue number
4 participants