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

Surface properties #2813

Merged
merged 12 commits into from
May 23, 2024
Merged

Surface properties #2813

merged 12 commits into from
May 23, 2024

Conversation

ScriptLineStudios
Copy link
Member

Closes #2758

@ScriptLineStudios ScriptLineStudios requested a review from a team as a code owner April 20, 2024 21:04
Starbuck5
Starbuck5 reviewed Apr 20, 2024
View reviewed changes
test/surface_test.py Outdated Show resolved Hide resolved
Starbuck5
Starbuck5 reviewed Apr 20, 2024
View reviewed changes
src_c/surface.c Outdated Show resolved Hide resolved
@Starbuck5 Starbuck5 added New API This pull request may need extra debate as it adds a new class or function to pygame Surface pygame.Surface labels Apr 20, 2024
ankith26
ankith26 reviewed Apr 21, 2024
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.

Interesting idea!

This PR is adding both .w and .width and .h and .height. Is this actually required? Is it being done for consistency with rect? If we add this we will end up having like atleast 3 ways of doing the exact same thing, which isn't very ideal IMHO

@ScriptLineStudios
Copy link
Member Author

Interesting idea!

This PR is adding both .w and .width and .h and .height. Is this actually required? Is it being done for consistency with rect? If we add this we will end up having like atleast 3 ways of doing the exact same thing, which isn't very ideal IMHO

Yeah I also felt it was a little redundant, but yes did it for consistency with Rect.

@Starbuck5
Copy link
Member

Let's just do width, height, and size for now. (Removing w and h).

I think w and h would be fine to have, but in the interest of making this PR easy to merge let's remove them for now.

They can always be added in the future, but once they're added it's quite hard to remove.

itzpr3d4t0r
itzpr3d4t0r approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@itzpr3d4t0r itzpr3d4t0r left a comment

Choose a reason for hiding this comment

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

Agree with previous suggestions, that aside LGTM!

@ScriptLineStudios
Copy link
Member Author

Let's just do width, height, and size for now. (Removing w and h).

I think w and h would be fine to have, but in the interest of making this PR easy to merge let's remove them for now.

They can always be added in the future, but once they're added it's quite hard to remove.

Sounds good!

ankith26
ankith26 reviewed Apr 26, 2024
View reviewed changes
docs/reST/ref/surface.rst Outdated Show resolved Hide resolved
novialriptide
novialriptide approved these changes May 18, 2024
View reviewed changes
Copy link
Member

@novialriptide novialriptide left a comment

Choose a reason for hiding this comment

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

I think everything here is good.

Starbuck5
Starbuck5 approved these changes May 21, 2024
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.

Andrew and I added some commits to get this fully up and good w/ Ankith's feedback, I'm still happy with it.

@oddbookworm oddbookworm requested a review from ankith26 May 23, 2024 01:28
@oddbookworm oddbookworm force-pushed the Surface-Attributes branch from 6373f08 to f4135f7 Compare May 23, 2024 01:34
ankith26
ankith26 approved these changes May 23, 2024
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. This is a nice simple change that should make things more convenient for users, thanks! 🍰

@ankith26 ankith26 added this to the 2.5.0 milestone May 23, 2024
@ankith26 ankith26 merged commit 8a0744d into pygame-community:main May 23, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itzpr3d4t0r itzpr3d4t0r itzpr3d4t0r approved these changes

@ankith26 ankith26 ankith26 approved these changes

@Starbuck5 Starbuck5 Starbuck5 approved these changes

@novialriptide novialriptide novialriptide approved these changes

Assignees
No one assigned
Labels
New API This pull request may need extra debate as it adds a new class or function to pygame Surface pygame.Surface
Projects
None yet
Milestone
2.5.0
Development

Successfully merging this pull request may close these issues.

Idea: start adding a small number of properties to Surface
6 participants
@ScriptLineStudios @Starbuck5 @novialriptide @ankith26 @itzpr3d4t0r @oddbookworm