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

Update tkinter.Text.count for 3.13 #12368

Closed
wants to merge 11 commits into from

Conversation

max-muoto
Copy link
Contributor

Add new return_ints argument, and add appropiate overloads.

return_ints: Literal[True],
) -> int: ...
else:
def count(self, index1: _TextIndex, index2: _TextIndex, *options: _CountOptions) -> tuple[int, ...]: ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old docstring claims this could return a list of integers, but that seems to be false.

@max-muoto max-muoto marked this pull request as draft July 20, 2024 04:53
@max-muoto max-muoto marked this pull request as ready for review July 20, 2024 05:04

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

if sys.version_info >= (3, 13):
@overload
def count(
self, index1: _TextIndex, index2: _TextIndex, *options: Unpack[tuple[_CountOptions]], return_ints: Literal[True]
Copy link
Member

Choose a reason for hiding this comment

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

If I'm reading the docstring right, this isn't quite correct: if one of the options is update, then that doesn't produce an extra value.

Typing this fully precisely seems difficult and possibly not worth the trouble.

cc @Akuli for opinions on tkinter.

Copy link
Collaborator

@Akuli Akuli Jul 21, 2024

Choose a reason for hiding this comment

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

This method is just awful to work with without the return_ints parameter. Before that parameter existed, I wrote my own just to avoid figuring out how it works.

That said, typing this method is probably worth the effort:

  • it is tempting to use .count() and easy to forget some corner case
  • most people can't assume Python 3.13 yet

I think I'll just write the overloads and push directly into this PR's branch. decided to make new PR

def count(
self, index1: _TextIndex, index2: _TextIndex, *options: Unpack[tuple[_CountOptions]], return_ints: Literal[True]
) -> int: ...
# If there are 2+ options, then a tuple of ints is always returned.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not true if you specify "update" and some other option. I'll make a new PR that adds overloads for this...

@Akuli
Copy link
Collaborator

Akuli commented Aug 10, 2024

This now has a merge conflict, but it should be much simpler now that #12395 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants