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.

Sign up for GitHub

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

Jump to bottom

gh-126081: For PRs labeled with "type-feature", require a core review #126082

Merged
merged 6 commits into from
Oct 28, 2024
Merged

gh-126081: For PRs labeled with "type-feature", require a core review #126082

merged 6 commits into from
Oct 28, 2024

Conversation

ambv
Copy link
Contributor

@ambv ambv commented Oct 28, 2024 •
edited by bedevere-app bot
Loading

This PR is the test for this PR.

@bedevere-app bedevere-app bot mentioned this pull request Oct 28, 2024
Open
@ambv ambv added the skip news label Oct 28, 2024
@ambv ambv added the type-feature A feature request or enhancement label Oct 28, 2024
@ambv ambv added type-feature A feature request or enhancement and removed type-feature A feature request or enhancement labels Oct 28, 2024
@ambv ambv requested review from hugovk and removed request for hugovk October 28, 2024 16:23
hugovk
hugovk approved these changes Oct 28, 2024
View reviewed changes
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.

🙈 I'm sure it's fine....

@ambv
Copy link
Contributor Author

ambv commented Oct 28, 2024 •
edited
Loading

Behavior without a type-feature label

Screenshot 2024-10-28 at 17 16 37 Screenshot 2024-10-28 at 17 21 49

Behavior with a type-feature label but no review

Screenshot 2024-10-28 at 17 19 48 Screenshot 2024-10-28 at 17 21 02

Behavior with a type-feature label and an accepting core review

Screenshot 2024-10-28 at 17 40 13 Screenshot 2024-10-28 at 17 27 21

Behavior with a type-feature label and a core review requesting changes

Screenshot 2024-10-28 at 17 43 11 Screenshot 2024-10-28 at 17 43 19

hugovk
hugovk requested changes Oct 28, 2024
View reviewed changes
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.

(Checking what request-changes looks like)

@bedevere-app
Copy link

bedevere-app bot commented Oct 28, 2024

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

ezio-melotti
ezio-melotti reviewed Oct 28, 2024
View reviewed changes
Copy link
Member

@ezio-melotti ezio-melotti left a comment

Choose a reason for hiding this comment

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

Can you add some inline comments to explain more clearly what each step does?

For example the "Check for type-feature" seem to only be used for the awaiting-merge check, whereas the "no required changes" can fail the build directly.

@ambv
Copy link
Contributor Author

ambv commented Oct 28, 2024

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Oct 28, 2024

Thanks for making the requested changes!

@hugovk: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from hugovk October 28, 2024 16:51
@hugovk
Copy link
Member

hugovk commented Oct 28, 2024

Looking good:

image

@ambv ambv enabled auto-merge (squash) October 28, 2024 16:56
@ambv ambv merged commit ced2691 into python:main Oct 28, 2024
39 of 42 checks passed
@ambv ambv deleted the gh-126081-phase1 branch October 29, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hugovk hugovk hugovk approved these changes

@ezio-melotti ezio-melotti ezio-melotti approved these changes

Assignees
No one assigned
Labels
skip news type-feature A feature request or enhancement
Projects
CPython labels
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ambv @hugovk @ezio-melotti