Improve typing

[bryanforbes]

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.

[Pike]

Just read the other day that a str is also an Iterable{str], we should avoid that type if we really want multiple strings.

[bryanforbes]

In a loosely typed language like Python, I'm not sure this can be avoided. There are two options, in my mind: use a union of a bunch of common invariant collections (Union[List[str], Tuple[str], AbstractSet[str]]) or add a runtime check to ensure that a string isn't being passed. I really don't like the first option because you're limiting what the user can pass in (a List[MyStringSubtype] can't be passed in, for instance), which is why I changed this to Sequence[str] and Iterable[str].

[bryanforbes]

A third option is to convert from str to a list() (which is done in a couple of other places):

    def __init__(
        self,
        locales: Union[str, Sequence[str]],
        resource_ids: Union[str, Iterable[str]],
        resource_loader: 'AbstractResourceLoader',
        use_isolating: bool = False,
        bundle_class: Type[FluentBundle] = FluentBundle,
        functions: Union['SupportsKeysAndGetItem[str, Callable[[Any], FluentType]]', None] = None,
    ):
        self.locales = [locales] if isinstance(locales, str) else list(locales)
        self.resource_ids = [resource_ids] if isinstance(resource_ids, str) else list(resource_ids)

[eemeli]

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?

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.
• 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"?

[eemeli]

Triplicating the cast is a bit much, especially as we already have the source value right there.

Suggested change

	if '.' in cast(str, self.value):
	self.value = FluentFloat(self.value)
	self.value = FluentFloat(cast(str, self.value))
	else:
	self.value = FluentInt(self.value)
	self.value = FluentInt(cast(str, self.value))
	if '.' in value:
	self.value = FluentFloat(value)
	else:
	self.value = FluentInt(value)

[bryanforbes]

Agreed. However, without the cast to str and with the change I made to FluentInt, it's trying to pass what it thinks is a Union[FluentFloat, FluentInt] to FluentInt() and marks it as an error. This should probably be cleaned up to something like this:

original_value = cast(str, self.value)
if '.' in original_value:
    self.value = FluentFloat(original_value)
else:
    self.value = FluentInt(original_value)

[eemeli]

The diff is a perhaps a bit misleading. The argument value is already properly recognised as a str, and it's assigned to self.value in the super().__init__(). So we can just use that directly when re-setting the self.value.

[bryanforbes]

I had thought of that but thought that manipulation of value could happen in the future in FTL.NumberLiteral or BaseResolver, so I didn't want to assume too much. Do you foresee that happening? If not, using value directly is the better solution.

[eemeli]

That sounds like it'd be excessive magic. Better to use value directly to add an extra hurdle for such shenanigans.

[eemeli]

So explicitly typing this and TIME_STYLE_OPTIONS below seems a bit excessive. Is there some actual benefit to doing so?

[bryanforbes]

You're right, and I made this change when I was making sure the API worked with Pyright. Mypy is a bit looser validating attr.ib() calls with validators, so the error doesn't show up there. Without the explicit typing, DATE_STYLE_OPTIONS is inferred as Set[Union[str, None]] and the dateStyle attribute definition is marked as an error by pyright because the validator checks for Union[str, None] which is wider than the attribute type of Union[Literal['full', 'long', 'medium', short'], None]. Since this error doesn't affect how pyright interprets the public API, I can revert this and just make it Final.

[eemeli]

That sounds good.

[Pike]

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.

[bryanforbes]

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.