Skip to content

Conversation

@Akuli
Copy link
Collaborator

@Akuli Akuli commented Aug 18, 2020

(If you have time to review only one PR, then please review #4459 instead, it's more important IMO)

I don't know how to get rid of overlapping dump overloads. I want it to return None if a non-None command is specified, and List[Tuple[str, str, str]] if command is not specified or it's specified as None. The problem is that index2 is before command, and that has a default value too.

tag_prevrange and tag_nextrange return a union. It's not great, but I think it's similar to returning Optional; if you forget to handle the two different cases, then you likely have a bug. And if you're sure that one of the two cases never happens, then you should assert that.

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 18, 2020

I think it's now correct:

  • If command is given as a positional argument, then index2 also has to be positional, so there's an overload that has both of them positional.
  • Giving command as a keyword argument can be done with another overload that has it keyword-only, and that way index2 can still have a default value (and it can be positional or keyword argument).

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 18, 2020

One more thing: text index strings are like "line.column", as in "123.4" for column 4 of line 123. They look like floats even though they aren't floats, and calculating with them as if they were floats is an error (for example, 1.2 > 1.10, even though column 2 comes before column 10).

However, tkinter accepts floats as well, and some people use float text indexes to "reduce typing", most commonly 1.0 to represent the beginning of the text widget. I was going to allow Literal[1.0] to be used for that, but you can't make a Literal of a float. On the other hand, allowing any float would also mean allowing any int, and 5 is not a valid text index, for example. What should we do?

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 20, 2020

@Hawk777 What do you think about float text indexes?

@Hawk777
Copy link
Contributor

Hawk777 commented Aug 20, 2020

I haven’t really used the Text class much. From looking at the documentation, it looks like indices are far more general than just number-dot-number. However, tkinter does accept floats, so I suppose the mypy annotations ought to indicate that—even if we really wish that floats didn’t work, because they’re ugly, the fact is that they do.

I’m not sure how relevant it is that comparisons don’t work right. Most math also wouldn’t work right, but I don’t think it’s the place of a type system to try and enforce rules about how the values are calculated. While some languages do use their type systems to indicate detailed constraints on values (e.g. Rust’s NonZeroU32 type, which is literally just a 32-bit integer that isn’t zero), that’s not really the mypy/Python way AFAICT, especially not in the Tk world where everything is strings.

Put another way, even if the type system accepts only strings, then you can pass so many different types of invalid values anyway. Given that, would prohibiting floats help much?

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 20, 2020

it looks like indices are far more general than just number-dot-number

Right, you can put different kinds of things into a string when specifying a text index. It's handy and I often do it, but it's not really related to the float thing.

you can pass so many different types of invalid values anyway. Given that, would prohibiting floats help much?

With these stubs, I make sure to accept two kinds of values:

  • Values that other methods return, such as Tcl_Obj text indexes and "line.column" strings.
  • Values that users create, such as non-"line.column" strings (and possibly floats).

I'm struggling with figuring out whether it's more practical to allow floats and ints, possibly allowing surprising bugs to happen, or disallow floats altogether, forcing lazy typers to add some quotes into code that used to pass mypy checks.

@Hawk777
Copy link
Contributor

Hawk777 commented Aug 20, 2020

My point is that, if you accept strings, then the type system will accept things like "42.hello", "/abcd-efgh_ijkl+mnop", and all sorts of other bad data. If you already accept a lot of bad data, is it a big deal if you also accept floats?

I suppose the question is: what is the philosophy of typeshed? Should the type annotations reflect all the ways in which the API can be legitimately used (in which case we must accept floats), or should they reflect only what we consider to be “good” ways (in which case we should probably reject floats)?

@Akuli
Copy link
Collaborator Author

Akuli commented Aug 20, 2020

floats allowed now because avoiding errors is generally the way to go in typeshed

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

No red flags.

@srittau srittau merged commit 770fe90 into python:master Aug 24, 2020
@Akuli Akuli deleted the tkinter-text branch August 24, 2020 11:35
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