-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
Auto add the skip news
label to docs
PRs
#485
Conversation
Codecov Report
@@ Coverage Diff @@
## main #485 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 18
Lines 1815 1829 +14
Branches 220 222 +2
=========================================
+ Hits 1815 1829 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Well, that's stupid of me. That Questions from my OP are still relevant though! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To answer your questions:
- if a PR has a news item, the label shouldn't be applied (see inline comment)
- if a PR has only tests, I also think it shouldn't be applied (at least not in this PR)
bedevere/prtype.py
Outdated
elif docs: | ||
await add_category(gh, issue, Category.documentation) | ||
await add_category(gh, issue, [Category.documentation, Category.skip_news]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you shouldn't apply the label in case of a PR which includes news. This check is already done a few lines above, but nothing is done with information. You could simply add a news = True
and only add the label if not news
.
Thanks for the speedy review 😄
Changed!
This is the status quo, so no further changes necessary I think. I merged #486 into this as I think that should probably be merged before this. I can rebase after that has been merged. |
Co-authored-by: Ezio Melotti <[email protected]>
Already rebased on #486 as I mixed up my branches, sorry about that. Consolidated all changes + review of this PR in a single commit. |
Co-authored-by: Ezio Melotti <[email protected]>
@ezio-melotti Your suggestions break one test. If have pushed a commit that fixes it, but could you look into whether this is correct? |
I fixed the merge conflicts and left another review, however I'm still not too convinced about the
bedevere/bedevere/filepaths.py Lines 12 to 21 in bf7a01b
(click to see the definition of `gh`)Lines 35 to 37 in 83a2d22
It then claim to categorize a PR: Lines 29 to 30 in 83a2d22
but first it retrieves the issue linked to the PR: Line 37 in 83a2d22
(or at least that's what it seems to do -- click to see source)Lines 90 to 92 in 83a2d22
then it passes the Lines 49 to 52 in 83a2d22
and Lines 22 to 26 in 83a2d22
So it seems to me that the intention is to update the PR, but -- unless something is misnamed and Can someone more familiar with bedevere confirm? |
I think the confusion comes from this function (which is poorly named imo). It's based on the following payload for As you can see Rather than |
That makes sense! So in Lines 90 to 92 in 83a2d22
It first gets the API URL from the
Maybe just something like
I think the whole test should be rewritten. see test_leave_existing_type_labels sourceLines 155 to 172 in 83a2d22
It seems that the goal of the test is checking that if a PR already has a It could however be replaced by a new test that checks that if an existing label doesn't get reapplied, e.g. if a PR already has a Footnotes
|
I looked into a refactoring PR, but it seems like this terminology is being used throughout the code base. Take for example these lines: Lines 61 to 67 in 83a2d22
Again
Rewritten and added a new/old test to bump coverage again. We now check that 1) we don't post if there is an exact match with current labels, 2) we "partially" post if there is a partial match with the current labels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new tests look good. I see what you are saying about the refactoring too.
I left a comment about the docstring, but I agree we shouldn't rename the function in this PR.
bedevere/util.py
Outdated
"""Get the issue_url for a pull request. | ||
|
||
The issue_url is the API endpoint for the given pull_request. | ||
This terminology follows the official Github API and its documentation. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this doesn't just get the URL of the API endpoint, but (unless I'm misunderstanding again) it also retrieves the JSON data from that endpoint, returning a dict that contains the info/data for the PR (hence my suggestion about calling it get_pr_info
/get_pr_data
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad. You're right. What about:
"""Get the issue_url for a pull request. | |
The issue_url is the API endpoint for the given pull_request. | |
This terminology follows the official Github API and its documentation. | |
""" | |
"""Get the issue_url for a pull request. | |
The issue_url is a JSON payload with all data relevant for the given pull_request. | |
This terminology follows the official Github API and its documentation. | |
""" |
Co-authored-by: Ezio Melotti <[email protected]>
For future reference, this was the first PR in which the new labeling worked: python/cpython#94828. |
Closes #457.
Also closes python/core-workflow#363.
Follow up can also tackle python/core-workflow#421 but I decided to keep this small since the feedback on python/core-workflow#363 shows that the consensus reached in #457 is not shared by everybody.
Two edge cases which were not considered in the original issue but on which somebody should probably take a decision:
skip news
label as shown bytest_docs_and_news
.skip news
label also be applied if the PR has a test? Currently I thought I would not do so as to not make this change too controversial, but I'm happy to change this obviously.Let me know what you think!