ui.download signature is unintuitive

[marcuslimdw]

Description

This is the ui.download API:

def download(src: Union[str, Path, bytes], filename: Optional[str] = None, media_type: str = '') -> None:

If src is of type bytes, then it represents the raw contents of the file. However, if it is of type str or Path, then we check if it identifies a valid file; if so, we mount that as a static file. Otherwise, we assume that it is a URL, and send the client to that.

This is confusing because most APIs throughout the Python ecosystem (that I've seen, at least) that take str | bytes do so understanding that the user may bring their own encoding (and use str), or not (and use bytes), and so, semantically, the two represent the same thing.

On the other hand, here, str means "file path or URL", and bytes means file contents, which is super unintuitive IMO - a coworker recently wanted to trigger a download of a JSON object that happened to be a str, and spent a fair bit of time wondering why it wouldn't work.

The most common way I see to deal with this is to have two separate APIs - one that takes a path, and one that takes a body (e.g. etree.open vs etree.fromstring). An alternative is to have separate keyword arguments, something like source_path and contents, and to raise an error if both are specified.

[falkoschindler]

Thanks for bringing this up, @marcuslimdw!

We definitely see your point. The API has been growing over the past:

1. Initially ui.download expected a URL (str).
2. Than we simplified downloading files by automatically serving them if there's a file with the given path (str or Path).
3. Last but not least we simplified downloading given file content (bytes).

The first two points try to hide the problem of serving files before being able to request them. Here NiceGUI wraps underlying server-client concepts that should be irrelevant for a UI framework. But too much implicit magic comes at a cost, for sure.

Given the current API, how would you migrate towards a better, more explicit solution?

I could imagine

• keeping the positional argument src: str | Path for the most common case: auto-serving local files,
• adding extra keyword arguments url: str and content: str | bytes,
• printing a deprecation warning when a misuse is detected, and
• changing the warning to an exception in version 3.0.

[marcuslimdw]

IMO, that's not the best solution, because it makes it possible to represent invalid states (what should happen if both src and content are specified?), which means extra code is needed to check for all the combinations.

I suggest instead the following changes:

1. ui.download, instead of being a function, becomes an instance of a new Download class.
2. The Download class has three methods: from_local_file, from_url, and from_bytes, each of which has one argument of the appropriate types.
3. To maintain backwards compatibility, the Download class also has a __call__ method with the same parameters as the present ui.download, but which becomes deprecated starting from a point of your choice.

This makes it practically impossible to do the wrong thing, while making the API self-documenting, since the type of input you're supposed to provide is right in the method name. Usage would be as follows:

# old
ui.download(Path(__file__) / "data.json")

# new
ui.download.from_local_path(Path(__file__) / "data.json"))

As an aside, this can also ease testing, because this new Download class and the object that currently replaces ui.download can share a common interface, which makes type checking easier and ensures implementation-test parity.

[falkoschindler]

That's a great idea, @marcuslimdw! This provides a really clean separation between the old and the three new ways to use ui.download.

What do you think about the shorter names

• ui.download.file,
• ui.download.url, and
• ui.download.content?

The term "content" would include both strings as well as bytes. Or would ui.download.data be even better?

[marcuslimdw]

yes, you're right - "content" is a more appropriate term than "bytes" (I forgot that with this, we'd be able to allow the user to download strings too). other than that, I have a slight preference for "from_url instead of url, because then all three versions read like English, but your other two changes sound great to me!

[falkoschindler]

Good point, @marcuslimdw. For consistency I didn't want to add from_ just for one of the three methods. But you're right, we don't download a URL but from a URL. So this would be the new API:

• ui.download() (deprecated)
• ui.download.file()
• ui.download.from_url()
• ui.download.content()

Would you like to create pull request? 🙂

[marcuslimdw]

@falkoschindler honestly I don't use NiceGUI much any more, but I don't mind creating a PR for this - it may take a while, though!