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

Expand __init__ parameters for Window #3295

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

damusss
Copy link
Member

@damusss damusss commented Jan 17, 2025

It's a bit annoying having to open the docs every time to remember if a flag exists or not and since this are bool flags and not int flags I think this adds clarity. Additionally, when you start typing a flag your IDE will suggest it to you and you'll spell it right.

Finally I used ... instead of False because the flag doesn't really enable or disable the feature, it justs sets the flag for SDL to handle. the default for that feature might be True in that specific os, so False would be incorrect, imo. Many libraries I saw used this signature, but I can change it if it's not liked.

I used the docs to get the flags, if some flags are to be deprecated for SDL3 I can omit them, of course.

@damusss damusss added type hints Type hint checking related tasks window pygame.Window labels Jan 17, 2025
@damusss damusss requested a review from a team as a code owner January 17, 2025 20:26
@aatle
Copy link
Contributor

aatle commented Jan 20, 2025

I like the idea, but I am not sure about the fact that the overload is not matchable and an error has to be ignored. If we put this overload first or replace the original, then it may be annoying as you've said.
API-wise, ignoring user experience, it doesn't make sense to have two separate overloads.

Also, for reference, here's the "wall of text" that the user would see after this change for this overload (autocomplete kwargs would be filtered as the name is being typed).
window-lsp
I've seen larger ones, though.

class transformers.AlbertConfig(vocab_size=30000, embedding_size=128, hidden_size=4096, num_hidden_layers=12, num_hidden_groups=1, num_attention_heads=64, intermediate_size=16384, inner_group_num=1, hidden_act='gelu_new', hidden_dropout_prob=0, attention_probs_dropout_prob=0, max_position_embeddings=512, type_vocab_size=2, initializer_range=0.02, layer_norm_eps=1e-12, classifier_dropout_prob=0.1, pad_token_id=0, bos_token_id=2, eos_token_id=3, **kwargs)

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 for the PR 🎉

allow_high_dpi: bool = ...,
mouse_capture: bool = ...,
always_on_top: bool = ...,
utility: bool = ...,
) -> None: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
) -> None: ...
**flags: bool,
) -> None: ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, why would that be necessary? any flag that is not in the current parameter list will result in an error. it seems to me the code you are suggesting doesn't reflect what's going on in C. if a new flag will be added, a new keyword parameter will be added too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. I was thinking about the original signature and assumed that it was supported.
If flags are to be deprecated though, will they be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it too, like, "will this make it harder to remove things?" not really. what is in the docs is official and public. just because it wasn't in the stubs before doesn't mean it could just be removed. if a flag is deprecated, the keyword parameter will be removed. your linter will complain, but your code won't fail, it'll just output a warning. the linting failing is actually good, because it discourages usage, just like @deprecated does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Disallowing deprecated parameters in typing is not appropriate.
Either, the deprecated parameters are still compatible with typing via the **kwargs parameter, or there is a deprecated overload for them (possibly using **kwargs). There might also be runtime warnings to accompany them.

Copy link
Member Author

Choose a reason for hiding this comment

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

the warning are sure. how would the overload be compatible with mypy? should it be added now? perhaps when deprecating the keyword argument will not be removed, but it could be typehinted in some special way, I hope so. Either way, I believe it to be slightly outside of scope of this PR. good for noticing tho :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.
When a flag is deprecated, we can either keep it in the signature or create a separate deprecated overload.

Thanks for clarifying this.

Copy link
Contributor

@aatle aatle 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!

Copy link
Contributor

@bilhox bilhox left a comment

Choose a reason for hiding this comment

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

After reading all messages and the content of the code changes, I can say that it looks good to me.
Thanks for the PR !

@damusss damusss changed the title Expand init parameters with overload for Window Expand __init__ parameters for Window Jan 20, 2025
@ankith26 ankith26 added this to the 2.5.3 milestone Jan 21, 2025
@ankith26 ankith26 merged commit 645cda1 into pygame-community:main Jan 21, 2025
24 checks passed
@damusss damusss deleted the window-expand-init-stubs branch January 23, 2025 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type hints Type hint checking related tasks window pygame.Window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants