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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more type annotations #4657

Merged
merged 16 commits into from May 3, 2022
Merged

Add more type annotations #4657

merged 16 commits into from May 3, 2022

Conversation

harahu
Copy link
Contributor

@harahu harahu commented Apr 27, 2022

馃摎 Context

Add some low hanging type annotations to the code base.

I run mypy with fairly strict settings, including https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-disallow-untyped-calls

This results in a lot of errors for various parts of the Streamlit API. This PR adds type annotations to a few APIs I use heavily, in order to silence those errors.

  • What kind of change does this PR introduce?
    • Other, please describe: Type annotations

馃 Description of Changes

  • Add type annotations to various parts of the Streamlit API.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@harahu
Copy link
Contributor Author

harahu commented Apr 29, 2022

I'd love to add more annotations, but I'd like to know if this is something you guys find valuable first.

Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @harahu!

We certainly find this valuable and would appreciate additional PRs adding more annotations 馃檪

I have a few small comments / one question, but for the most part this looks close to being ready to merge to me

lib/streamlit/elements/alert.py Outdated Show resolved Hide resolved
lib/streamlit/elements/markdown.py Show resolved Hide resolved
@harahu
Copy link
Contributor Author

harahu commented May 3, 2022

Thanks for the contribution @harahu!

We certainly find this valuable and would appreciate additional PRs adding more annotations slightly_smiling_face

Good to know! Will split my work over multiple PRs then, in order to maintain ease of review and merging. This is implying that I won't be adding more annotations to this PR, so feel free to merge it as you feel like.

Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

Thanks again @harahu!

@vdonato vdonato merged commit 27ff5c2 into streamlit:develop May 3, 2022
tconkling added a commit that referenced this pull request May 3, 2022
* develop:
  Add more type annotations (#4657)
  CI release flow improvements (#4661)
  Cloud updates in README (#4668)
  Fix moment.js timezone error (#4669)
@harahu harahu deleted the harahu-types branch May 4, 2022 16:52
@harahu
Copy link
Contributor Author

harahu commented May 4, 2022

@vdonato I've added a small heap of PRs addressing typing for various APIs. Referring to #4679 through #4691. Most of them are quite small an self-contained, so dig though them at your own pace.

@vdonato
Copy link
Collaborator

vdonato commented May 5, 2022

@harahu I've gone ahead and approved + merged about 5 of the newly opened PRs. Another one is more or less good to go as well, but I had one small suggestion on it.

There's a bit of a wrinkle in getting the rest of the PRs merged that we'll need to figure out how to resolve. I'm not sure how this slipped by CI as I would have expected e2e tests to fail due to this (my best guess right now is that we somehow end up caching typing_extensions so that it's installed on Python >= 3.8 in CI, but I'll have to confirm that this is actually what's going on).

The issue is that we conditionally install the typing_extensions package as a dependency depending on the user's Python version, but this PR made a change importing it unconditionally, which resulted in a runtime error when using streamlit with Python >=3.8. I fixed this in #4692, but I wonder if it would make more sense at this point to simply always install typing_extensions rather than adding more of these conditional imports since I doubt there's much overhead/risk to adding the package as a dependency for all Python versions.

(CC @AnOctopus @tconkling as they're likely to have opinions on how to best resolve this)

@harahu
Copy link
Contributor Author

harahu commented May 5, 2022

The issue is that we conditionally install the typing_extensions package as a dependency depending on the user's Python version, but this PR made a change importing it unconditionally, which resulted in a runtime error when using streamlit with Python >=3.8.

I'm sorry! Wasn't aware that it was a conditional install.

I fixed this in #4692, but I wonder if it would make more sense at this point to simply always install typing_extensions rather than adding more of these conditional imports since I doubt there's much overhead/risk to adding the package as a dependency for all Python versions.

@vdonato There shouldn't be much risk, I think. It's also worth noting that the typing module as it is for python 3.8 is not the final iteration. More useful types have been added since (e.g. Annotated, ParamSpec), and more will be added in the future. Having typing_extensions available also for later versions of python means it is easier to keep the typing modern, as 3.8 grows older.

There's a remaining question about how to avoid types needlessly being imported from typing_extensions. This will be relevant every time you drop support for an older version of Python. Your solution, using a branching import, at least makes it clear to a reader when an import can be upgraded (in the case of Final that would be when the minimum version >= 3.8). There might be nothing wrong with combining this pattern with typing_extensions being an unconditional import.

@harahu
Copy link
Contributor Author

harahu commented May 5, 2022

Update: I've now added branching imports to the remaining PRs. That means some of them are ready to merge, while others, using the TypeAlias type are dependent on typing_extensions being available for python versions <= 3.9.

@tconkling
Copy link
Contributor

The issue is that we conditionally install the typing_extensions package as a dependency depending on the user's Python version, but this PR made a change importing it unconditionally, which resulted in a runtime error when using streamlit with Python >=3.8. I fixed this in #4692, but I wonder if it would make more sense at this point to simply always install typing_extensions rather than adding more of these conditional imports since I doubt there's much overhead/risk to adding the package as a dependency for all Python versions.

I've been confused by this conditional installation - and my feeling is that yeah, let's just always install typing_extensions no matter what! We could just do that in a separate PR now, if it makes things easier?

@tconkling
Copy link
Contributor

(Also, @harahu - thanks so much for this work! Big fan of explicit typing.)

@harahu
Copy link
Contributor Author

harahu commented May 5, 2022

I've been confused by this conditional installation - and my feeling is that yeah, let's just always install typing_extensions no matter what! We could just do that in a separate PR now, if it makes things easier?

If you do that, then all my PRs will be ready to merge, so am all for it 馃槈

@harahu
Copy link
Contributor Author

harahu commented May 5, 2022

(Also, @harahu - thanks so much for this work! Big fan of explicit typing.)

The pleasure is mine! I love Streamlit, but it's going to be nice to get rid of my custom typed wrappers of your API...

@harahu
Copy link
Contributor Author

harahu commented May 6, 2022

@tconkling I made and additional PR #4697, making typing-extensions an unconditional dependency. I hope this is helpful.

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.

None yet

3 participants