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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve typing annotations for file_uploader and text_input #6371

Merged
merged 10 commits into from
Mar 29, 2023
Merged

Improve typing annotations for file_uploader and text_input #6371

merged 10 commits into from
Mar 29, 2023

Conversation

schaumb
Copy link
Contributor

@schaumb schaumb commented Mar 27, 2023

📚 Context

Complete some python typing annotation

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe: Typing completion

🧠 Description of Changes

  • Add typing information for file_uploader not overloaded version

  • Replace text_input function type argument annotation from str to Literal

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

.

Current:

.

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Contribution License Agreement

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

@lukasmasuch
Copy link
Collaborator

@schaumb Thanks for improving our typing :) Our Python tests are failing since NoneType is not part of types. I believe you can just use None here.

One additional small suggestion, maybe we can update the docstring here from str to "default" or "password":

https://github.com/streamlit/streamlit/pull/6371/files#diff-18bbc9b3c1a4eac342c607b062ca145dcbbcd95ee05eb319d2b67627f14f1b69R127

@schaumb
Copy link
Contributor Author

schaumb commented Mar 28, 2023

Thanks @lukasmasuch for the review.

I replaced NoneType with None and the docstring as you recommended.

@lukasmasuch lukasmasuch changed the title Complete typing annotations Improve typing annotations for file_uploader and text_input Mar 28, 2023
@lukasmasuch
Copy link
Collaborator

@schaumb Seems like the internal function _file_uploader also requires the return type annotation and probably also a cast:

https://github.com/streamlit/streamlit/actions/runs/4547249875/jobs/8017434164?pr=6371#step:10:17

You can see an example for casting in some of our other widgets, e.g.:

streamlit/lib/streamlit/elements/slider.py

Line 630 in a87132b

return cast(SliderReturn, widget_state.value)

@schaumb
Copy link
Contributor Author

schaumb commented Mar 28, 2023

@schaumb Seems like the internal function _file_uploader also requires the return type annotation and probably also a cast:

https://github.com/streamlit/streamlit/actions/runs/4547249875/jobs/8017434164?pr=6371#step:10:17

You can see an example for casting in some of our other widgets, e.g.:

streamlit/lib/streamlit/elements/slider.py

Line 630 in a87132b

return cast(SliderReturn, widget_state.value)

Thanks again @lukasmasuch

I resolve this issue too.

@lukasmasuch
Copy link
Collaborator

lukasmasuch commented Mar 28, 2023 •
edited
Loading

Oh, I think I was actually wrong here. Python does already know about the type of widget_state.value and we don't need a cast in this case.

And I think you can actually replace Union[UploadedFile, List[UploadedFile], None] with the SomeUploadedFiles which is already defined here:

https://github.com/streamlit/streamlit/pull/6371/files#diff-558ed3325bcdecbec4962268f1f8ab96dbd12a34bc72340e81bbf000c6536d5dR43

@schaumb
Copy link
Contributor Author

schaumb commented Mar 29, 2023

Done @lukasmasuch

@schaumb
Copy link
Contributor Author

schaumb commented Mar 29, 2023

ImportError: cannot import name 'Literal' from 'typing'
Literal is '3.8'. So it will include from typing_extensions

lukasmasuch
lukasmasuch approved these changes Mar 29, 2023
View reviewed changes
Copy link
Collaborator

@lukasmasuch lukasmasuch left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM 👍

@lukasmasuch lukasmasuch merged commit 5cb4cb6 into streamlit:develop Mar 29, 2023
@schaumb schaumb deleted the patch-1 branch March 29, 2023 10:55
tconkling added a commit to tconkling/streamlit that referenced this pull request Mar 29, 2023
@tconkling
Merge branch 'develop' into tim/StubOutStreamlitView
c673bc4 
* develop:
  StreamlitEndpoints.buildAppPageURL (streamlit#6386)
  Rename query param ?testing to ?_stcore_testing (streamlit#6392)
  supress warning when call `get_script_run_ctx` from `gather_metrics` decorator (streamlit#6384)
  Improve typing annotations for file_uploader and text_input (streamlit#6371)
  Add support for Pandas 2.0 (streamlit#6378)
  Remove UriUtil.buildMediaUri (streamlit#6379)
  Allow pytest unit test to be discoverable via IDE v2 (streamlit#6374)
  Allow session state to only allow serializable items (streamlit#6165)
eric-skydio pushed a commit to eric-skydio/streamlit that referenced this pull request Dec 20, 2023
@schaumb @eric-skydio
Improve typing annotations for file_uploader and text_input (streamli…
2c5bc51 
…t#6371)

* Add text_input 'type' parameter type hints

* Add file_uploader return value type hint

* Add missing import

* Enable any sequence on `file_uploader` type

* Replace NoneType to None

* Change `text_input` "type" parameter docstring

* Add `file_uploader` return cast

* Add type hint and cast to `_file_uploader`

* Update file_uploader.py

* Fix import
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Mar 22, 2024
@schaumb
Improve typing annotations for file_uploader and text_input (streamli…
d6ec71b 
…t#6371)

* Add text_input 'type' parameter type hints

* Add file_uploader return value type hint

* Add missing import

* Enable any sequence on `file_uploader` type

* Replace NoneType to None

* Change `text_input` "type" parameter docstring

* Add `file_uploader` return cast

* Add type hint and cast to `_file_uploader`

* Update file_uploader.py

* Fix import
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
@schaumb
Improve typing annotations for file_uploader and text_input (streamli…
c5fa561 
…t#6371)

* Add text_input 'type' parameter type hints

* Add file_uploader return value type hint

* Add missing import

* Enable any sequence on `file_uploader` type

* Replace NoneType to None

* Change `text_input` "type" parameter docstring

* Add `file_uploader` return cast

* Add type hint and cast to `_file_uploader`

* Update file_uploader.py

* Fix import
sfc-gh-pchiu pushed a commit to sfc-gh-pchiu/streamlit that referenced this pull request Sep 3, 2024
@schaumb @sfc-gh-pchiu
Improve typing annotations for file_uploader and text_input (streamli…
86e9651 
…t#6371)

* Add text_input 'type' parameter type hints

* Add file_uploader return value type hint

* Add missing import

* Enable any sequence on `file_uploader` type

* Replace NoneType to None

* Change `text_input` "type" parameter docstring

* Add `file_uploader` return cast

* Add type hint and cast to `_file_uploader`

* Update file_uploader.py

* Fix import
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasmasuch lukasmasuch lukasmasuch approved these changes

Assignees
No one assigned
Labels
security-assessment-completed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@schaumb @lukasmasuch