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 metric #4705

Merged
merged 1 commit into from May 10, 2022

Conversation

harahu
Copy link
Contributor

@harahu harahu commented May 8, 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 metric 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.

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.

LGTM --

We're technically barred from removing the str cast for the return type of st.metric now for backwards compatibility reasons. On the other hand I'd be shocked if someone were actually relying on the current st.metric return value, so we may be able to bend the rules here a bit and change its return type.

I'd prefer doing this because I just can't see how the current behavior is intentional / how anyone could possibly be relying on it.

@harahu
Copy link
Contributor Author

harahu commented May 10, 2022

I'd prefer doing this because I just can't see how the current behavior is intentional / how anyone could possibly be relying on it.

Due to the tiny, but still existent, chance that this might be intentional, I suggest merging this PR as is, and I can make a follow-up pretty rapidly that changes the return type to DeltaGenerator. I prefer doing it that way since it allows for easily reverting the breaking change, should it turn out controversial. It also allows discussing it with other parties before merge, without holding up the types.

@vdonato
Copy link
Collaborator

vdonato commented May 10, 2022

Due to the tiny, but still existent, chance that this might be intentional, I suggest merging this PR as is

Seems reasonable to me! I'll go ahead and merge this now.

I double-checked the (internal) spec that we wrote for this and couldn't find any mention of the return value for st.metric being any different from other non-widget elements. That paired with the fact that I can't see how a user would possibly want this behavior makes me lean toward believing that the current return value is simply an error. I'm happy to take the change correcting this return value on 馃檪

@vdonato vdonato merged commit 8d083f4 into streamlit:develop May 10, 2022
@harahu
Copy link
Contributor Author

harahu commented May 10, 2022

I'm happy to take the change correcting this return value on 馃檪

I'm more than happy to leave it to you, if you want to take it on. I appreciate the thorough follow-up!

tconkling added a commit that referenced this pull request May 11, 2022
* develop:
  Improve exception formatting with rich (#4294)
  Fix st.metric return value (#4716)
  Add type annotations for metric (#4705)
  Add type annotations for image (#4708)
@harahu harahu deleted the harahu/types/metric branch May 12, 2022 06:30
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