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

added keyword use_values to collidedict and collidedictall #2309

Merged
merged 8 commits into from
Aug 21, 2023
Merged

added keyword use_values to collidedict and collidedictall #2309

merged 8 commits into from
Aug 21, 2023

Conversation

oddbookworm
Copy link
Member

Currently, the second argument to collidedict and collidedictall for Rect and FRect is position-only. This enables using it as a kwarg

@oddbookworm oddbookworm added the rect pygame.rect label Jul 13, 2023
@oddbookworm oddbookworm requested a review from a team as a code owner July 13, 2023 02:40
@Starbuck5
Copy link
Member

Is this consistent with what upstream pygame has done with rect kwargs on those functions?

@oddbookworm
Copy link
Member Author

Is this consistent with what upstream pygame has done with rect kwargs on those functions?

upstream calls the kwarg values, but both our docs and upstream's docs say the kwarg should be use_values

@Starbuck5
Copy link
Member

The code overrules the docs on what's correct, so lets call the kwarg on our end values as well, but also have that correctly documented.

@oddbookworm
Copy link
Member Author

The code overrules the docs on what's correct, so lets call the kwarg on our end values as well, but also have that correctly documented.

Should be fixed

ankith26
ankith26 approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@ankith26 ankith26 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 👍

Starbuck5
Starbuck5 reviewed Jul 18, 2023
View reviewed changes
docs/reST/ref/rect.rst Outdated Show resolved Hide resolved
Starbuck5
Starbuck5 approved these changes Jul 18, 2023
View reviewed changes
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Thanks!

I left two comments that could potentially result in changes before a merge.

@oddbookworm oddbookworm added the Awaiting Merge For PRs that have at least one approving review and can be merged on subsequent reviews label Jul 26, 2023
@ankith26 ankith26 added Not complete Still needs some work and removed Awaiting Merge For PRs that have at least one approving review and can be merged on subsequent reviews labels Jul 29, 2023
@oddbookworm
Copy link
Member Author

force push due to needing to rebase

ankith26
ankith26 reviewed Aug 15, 2023
View reviewed changes
src_c/rect_impl.h Outdated Show resolved Hide resolved
@MyreMylar MyreMylar added this to the 2.3.2 milestone Aug 15, 2023
ankith26
ankith26 approved these changes Aug 19, 2023
View reviewed changes
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM after latest changes

@ankith26 ankith26 merged commit 610e9e3 into pygame-community:main Aug 21, 2023
@ankith26 ankith26 removed this from the 2.3.2 milestone Sep 2, 2023
@ankith26 ankith26 added this to the 2.4.0 milestone Sep 2, 2023
@oddbookworm oddbookworm deleted the rect_kwargs branch December 21, 2023 03:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Starbuck5 Starbuck5 Starbuck5 approved these changes

@ankith26 ankith26 ankith26 approved these changes

Assignees
No one assigned
Labels
Not complete Still needs some work rect pygame.rect
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@oddbookworm @Starbuck5 @ankith26 @MyreMylar