python-fluent icon indicating copy to clipboard operation
python-fluent copied to clipboard

Improve typing

Open bryanforbes opened this issue 2 years ago • 2 comments

I've done the following:

  • Bumped the minimum version of typing_extensions in order to use Self
  • Updated workflows to use mypy 1.1.1 to type check the project
  • Updated parameters to use protocols (Mapping, Sequence, Iterable, etc.) where possible
  • Marked constants as Final
  • Improved typing of merge_options() so it can be used by consumers
  • Added overloads for fluent_number() and fluent_date()
  • Used Iterator[] for iterators rather than the more complex Generator[]
  • Ran isort on fluent.syntax and fluent.runtime

There are probably other things that could be done:

  • Anything that inherits from SyntaxNode could have its __init__ typing improved by adding a properly typed span keyword-only parameter, but I'm not sure if splitting that out from kwargs is very beneficial for the codebase.
  • Technically, typing_extensions isn't being used for anything but typing annotations, so it's not really needed as a runtime dependency. However, that would require only importing from typing_extensions in if TYPE_CHECKING: blocks, but that would require everything that uses those imports to wrap them in strings (since Python 3.6 is still being supported, from __future__ import annotations can't be used).
  • I took a look at getting this to also pass type checking with pyright, but wasn't sure if that was desired. The public API works fine with pyright, so I wasn't too worried.
  • A typing extra could be added to fluent.runtime with types-babel as a requirement to make it easier for consumers to make sure they have all of the types for the public API.

Let me know if you'd like me to do any of the above.

bryanforbes avatar Mar 17 '23 01:03 bryanforbes

I share @Pike's concern about str also qualifying as an Iterable[str] as well as a Sequence[str], but this seems like a pretty common issue that doesn't really have a satisfactory solution?

https://patrick.cloke.us/posts/2023/02/24/python-str-collection-gotchas/ has some resolutions on their side.

Pike avatar Mar 20 '23 10:03 Pike

Thank you, this looks pretty great! A few nitpicky things inline.

I share @Pike's concern about str also qualifying as an Iterable[str] as well as a Sequence[str], but this seems like a pretty common issue that doesn't really have a satisfactory solution?

I'm not sure it's as big of an issue as it's made out to be. I replied to the concern with a couple of options.

Regarding your suggestions of additional next steps:

  • Improving the __init__ typing of SyntaxNode's span argument probably won't benefit actual users, so we can probably not bother with that.

👍

  • I considered the same re: typing_extensions, but came to the conclusion that the type quoting would get really rather unwieldy, and I think keeping it as a nominal runtime dependency until we can drop 3.6 support shouldn't cause too many problems.

I agree. Quoted types are super unwieldy.

  • Hearing that the public API satisfies pyright is good, and probably a good minimum on that front.

👍

  • Sorry if asking something obvious, but could you clarify what you mean by "A typing extra could be added to fluent.runtime"?

Adding an extras_require so users can do pip install fluent.runtime[typing] and get types-babel as well. Although now that I look at it, it seems that the APIs from babel that are used in fluent's public API are all typed, so this may not be needed.

bryanforbes avatar Mar 20 '23 13:03 bryanforbes