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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce media_type parameter for ui.download #2494

Merged
merged 1 commit into from
Feb 2, 2024

Conversation

falkoschindler
Copy link
Contributor

As requested in #2491, this PR allows to specify the media type when using ui.download.

Can be tested with the following snippet:

ui.button('Text', on_click=lambda: ui.download(b'', media_type='text/plain'))
ui.button('HTML', on_click=lambda: ui.download(b'', media_type='text/html'))

Chrome will automatically create a random file name with the extensions "txt" and "html", respectively.

@falkoschindler falkoschindler added the enhancement New feature or request label Feb 1, 2024
@falkoschindler falkoschindler added this to the 1.4.14 milestone Feb 1, 2024
@iron3oxide
Copy link
Contributor

Just a small note and I can add this myself if it's too much work, but I think it would make sense to add some typo-proofing here by constraining media_type to either a StrEnum or a set of Literals (as done in Language)

@falkoschindler
Copy link
Contributor Author

Thanks for the suggestion, @iron3oxide!
But given the sheer amount of media types, it could be hard to maintain and slow for some IDEs. Or do you have a rather small and stable subset in mind?

@me21
Copy link
Contributor

me21 commented Feb 1, 2024

Fastapi uses strings for media_type argument in its StreamingResponse.

@iron3oxide
Copy link
Contributor

iron3oxide commented Feb 1, 2024

My goodness, I had no idea that there were this many media types (even though arguably 99% seem somewhat obscure). I see three options:

  1. Keep it as it is and expect a string (errors might be a bit hard to track down but it's flexible and easy).
  2. Expect a "MediaType" StrEnum (with a subset) OR string, which would offer convenience/safety for those who want it, but flexibility for those who don't need that.
  3. Expect a "MediaType" StrEnum (from aenum) with a subset of media types and document that you can use extend_enum() to add members to "MediaType". This would introduce a new dependency (and might not even work, I'd have to test that), but it would be a strict constraint that can still be loosened if need be.

@falkoschindler
Copy link
Contributor Author

@iron3oxide

Regarding option 2: My experience is that a function like def f(s: Literal['Abc', 'Def'] | str) is reduced to def f(s: str) and VSCode won't help completing strings like "A...".

Option 3 sounds pretty cumbersome, to be honest.

So I tend towards FastAPI's solution with a plain string. In the end it's a rather advance feature. Usually you can specify a filename and let the browser infer the media type, covering most simple cases. And if you really need "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" as in #2491 (comment), you probably know what you're doing.

@iron3oxide
Copy link
Contributor

@falkoschindler Agreed, here's a fourth solution though:
Get the media types from the included mimetypes module and use the functional Enum API (or another method) to create a type with the included MIME types. No added dependency (no package at least) and I'd hope that a stdlib module is maintained properly.

I'm fine with putting this aside though and going with strings as it's not a big gripe, more a personal preference.

@falkoschindler
Copy link
Contributor Author

@iron3oxide Using the mimetypes module for a collection of relevant media types is an interesting approach. But let's keep it simple for now and use a plain str. 🙂

@falkoschindler falkoschindler merged commit 194a64a into main Feb 2, 2024
7 checks passed
@falkoschindler falkoschindler deleted the download-media-type branch February 2, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants