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

Use SequenceLike in geometry stubs #3212

Merged
merged 1 commit into from
Nov 17, 2024
Merged

Conversation

ankith26
Copy link
Member

@ankith26 ankith26 commented Nov 3, 2024

Separated out from #3143 for independent review

@ankith26 ankith26 requested a review from a team as a code owner November 3, 2024 10:05
@Starbuck5
Copy link
Member

So, I think a big part of the reason to separate out changes into separate PRs is so they can be merged at different times, not blocking each other. Are you planning to remove these changes from #3143?

Because right now this PR just blocks #3143, as far as I can tell. And #3211 just blocks #3128.

@ankith26
Copy link
Member Author

ankith26 commented Nov 3, 2024

This PR and #3211 are standalones that are not blocked. Once these are merged I will update the dependent PRs to unblock them

@Starbuck5
Copy link
Member

Yes, they are not blocked, but they block the PRs they were separated from. Which I feel defeats the point of splitting them out.

@ankith26
Copy link
Member Author

ankith26 commented Nov 3, 2024

Well, you cant split them out AND not block each other. In an ideal world this PR should have been merged before #3143 was even started, but that didn't happen. But I don't mind rebasing #3143 again so it's fine for now IG

@Starbuck5
Copy link
Member

Well, you cant split them out AND not block each other.

Why not?

These are two independent changes to the stubs.

Changing all instances of Tuple, Dict, List to tuple, dict, list
VS
Changing some instances from Sequence to SequenceLike.

@ankith26
Copy link
Member Author

ankith26 commented Nov 3, 2024

They are independent but do changes to the same line and git isn't smart enough to auto resolve those conflicts

Screenshot From 2024-11-03 16-16-56

@Starbuck5
Copy link
Member

So whichever PR merges first will graciously let the second PR deal with the merge conflict. That's fine.

@ankith26
Copy link
Member Author

ankith26 commented Nov 3, 2024

Yeah but I do have a preference for this one being merged first because it's ready and small. The other one needs a push either way to address your other review comments

@Starbuck5
Copy link
Member

I'm just saying the way you've set up these two PRs requires this to be merged first, when the changes are actually independent. I want to approve your other PR and not worry about this detail, let some geometry people take a look.

@yunline yunline added type hints Type hint checking related tasks geometry pygame.geometry labels Nov 4, 2024
Copy link
Contributor

@damusss damusss left a comment

Choose a reason for hiding this comment

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

Yeah I forgot to update this detail while porting them, thanks

Copy link
Member

@zoldalma999 zoldalma999 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@zoldalma999 zoldalma999 merged commit 71ca715 into main Nov 17, 2024
24 checks passed
@zoldalma999 zoldalma999 added this to the 2.5.3 milestone Nov 17, 2024
@ankith26 ankith26 deleted the ankith26-fix-sequence-stub branch November 18, 2024 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geometry pygame.geometry type hints Type hint checking related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants