django-ftl / fluent-compiler

High performance Python implementation of Fluent, Mozilla's l10n language
Other
21 stars 4 forks source link

Provide Type Annotations (PEP 484) #14

Open samylovma opened 10 months ago

samylovma commented 10 months ago

Reasons

Introduction of Type Annotations

References

spookylukey commented 10 months ago

Hi @Samylov-Mikhail thanks for opening the issue!

In general I think there is a good chance that type annotations and mypy checking (or maybe pyright) could be useful in this library, although I think they are not always helpful. It depends what you are using them for and the nature of the library.

In this case I've got a few questions for you:

All of the above questions are another way of asking "what problem are you hoping to solve?"

spookylukey commented 10 months ago

Sorry, I didn't mean to close!

samylovma commented 10 months ago

Hi, Luke. Thank you for the quick feedback!

What is your particular interest in fluent-compiler?

I'm using fluent-compiler directly (not using django-ftl) in my chat bot application. I have chosen this library because of better maintenance compared to fluent.runtime.

Which of the different uses of type hints concern you?

Mostly static type checking to detect issues with my use of the library.

Are you thinking from the perspective of a user of fluent-compiler or a prospective maintainer on the code base?

Mostly I thinking from the perspective of a user. Type hints can improve UX for developers like me who use fluent-runtime directly.

Since the library is stable, I guess type hints won't be enemies of a prospective maintainer. Otherwise we can create third-party type stubs.

Benefits of type hints for a maintainer:

Have you considered pyright instead of mypy?

Seriously no. I have no real experience with pyright. In mypy compared to pyright I like cool documentation and large community.

What kind of approach would you be thinking of to add types? Have you looked at https://github.com/Instagram/MonkeyType or similar?

Since the codebase isn't so large I'm thinking of adding type hints manually. But I don't mind trying automated tools for that.

spookylukey commented 10 months ago

That's sounds great - as long as you are a user and looking at changes that would benefit you as a user, that's great. My only concern is adding lots of stuff that may or may not be helpful. As a maintainer, there are definitely bits I know of that could benefit from types, but there are also bits that would be very hard to type and I wouldn't want you getting stuck with something too ambitious!

samylovma commented 10 months ago

Thank you for your rationality, @spookylukey! I suggest just starting to integrate mypy and add type hints gradually where it's real helpful. What do you think about this approach?

spookylukey commented 10 months ago

Thank you for your rationality, @spookylukey! I suggest just starting to integrate mypy and add type hints gradually where it's real helpful. What do you think about this approach?

That sounds fine to me - thanks in advance!

samylovma commented 10 months ago

Good!

So I have a plan:

  1. Do some refactoring before the next step. (I will create an issue or a PR for every point.)
    • 19

    • 21

    • Drop support for Python 3.6.
    • Exclude some modules from the package's public API.
    • Fix all warnings in the tests.
    • ...
  2. Get mypy to run successfully.
  3. Add type hints for:
    • ...

P. S. This plan can be updated.

UPDATE: This plan moved to the body of this issue.

spookylukey commented 10 months ago

Remove the ast_compat module.

Is there a reason for this? What particularly problem would removing it solve? The Python AST is not stable, and I expect it will continue to have changes, and having a single place to accommodate those changes has been very useful in the past.

samylovma commented 10 months ago

@spookylukey, I've created the issue #19 for that.