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 type annotations for plotly_chart #4684

Merged
merged 7 commits into from May 6, 2022

Conversation

harahu
Copy link
Contributor

@harahu harahu commented May 4, 2022

馃摎 Context

Continuing from where I left off in #4657

  • What kind of change does this PR introduce?

    • Other, please describe: Type Annotations

馃 Description of Changes

  • Add type annotations for the plotly_charts module

Contribution License Agreement

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

from typing import Final, Literal
else:
from typing_extensions import Final, Literal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vdonato I changed the imports here to be python version dependent. In the process I discovered that my used of TypeAlias actually does require typing_extensions to be installed almost unconditionally.

I thought I'd mention it as input to your decision process regarding that dependency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks yet again @harahu!

If we could bug you to make one more batch of changes to these PRs, I think that now that typing_extensions is always installed, we might as well remove the branching imports here (I'll make a quick PR removing them in the ~2 places where they already exist, so no need to worry about those).

You mentioned in a comment in another PR that having these branching imports even with typing_extensions always installed may be reasonable as it gives us a signal as to when the imports can be made from the main typing package, but I feel like this isn't worth the added complexity. I also feel like it'll most likely never be problematic if we entirely forget to change these imports to be from typings > typing_extensions as we drop support for older versions of Python (and if types get removed entirely from typing_extensions, we'll catch it in CI).

I'll go ahead and approve the currently open PRs and can merge them as these last changes are made.

from typing import Final, Literal
else:
from typing_extensions import Final, Literal

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks yet again @harahu!

If we could bug you to make one more batch of changes to these PRs, I think that now that typing_extensions is always installed, we might as well remove the branching imports here (I'll make a quick PR removing them in the ~2 places where they already exist, so no need to worry about those).

You mentioned in a comment in another PR that having these branching imports even with typing_extensions always installed may be reasonable as it gives us a signal as to when the imports can be made from the main typing package, but I feel like this isn't worth the added complexity. I also feel like it'll most likely never be problematic if we entirely forget to change these imports to be from typings > typing_extensions as we drop support for older versions of Python (and if types get removed entirely from typing_extensions, we'll catch it in CI).

I'll go ahead and approve the currently open PRs and can merge them as these last changes are made.

@vdonato vdonato merged commit 35e90bc into streamlit:develop May 6, 2022
tconkling added a commit that referenced this pull request May 10, 2022
* develop: (21 commits)
  Remove branching imports and always import from typing_extensions (#4699)
  Add type annotations for media (#4706)
  Components adjustments (#4694)
  Type annotations for type_util (#4704)
  Add type annotations for utils (#4703)
  Argument types for iframe (#4707)
  Add type annotations for write (#4686)
  Add type annotations for doc_string (#4690)
  Add type annotations for progress (#4688)
  Add type annotations for exception (#4681)
  Add type annotations for plotly_chart (#4684)
  Wrap Markdown text when there's no word break (#4696)
  Add type annotations for __init__.py (#4687)
  Make typing-extensions and unconditional dependency (#4697)
  Add type annotations for balloons (#4679)
  Add type annotations for empty (#4680)
  Add type annotations for json (#4682)
  Add type annotations for layouts (#4683)
  Add type annotations for snow (#4685)
  Import Final from typing or typing_extensions depending on the Python version (#4692)
  ...
@harahu harahu deleted the harahu/types/plotly-chart branch May 21, 2022 19:25
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

2 participants