Skip to content

os: add temporary notice about changing XDG user dirs related fns behavior#26301

Closed
gechandesu wants to merge 3 commits into
vlang:masterfrom
gechandesu:os_xdg_dirs
Closed

os: add temporary notice about changing XDG user dirs related fns behavior#26301
gechandesu wants to merge 3 commits into
vlang:masterfrom
gechandesu:os_xdg_dirs

Conversation

@gechandesu

Copy link
Copy Markdown
Contributor

Part 1 of #26300

Added a notice about the behavior change for now. I plan to change the behavior in another PR in a month.

@JalonSolov

Copy link
Copy Markdown
Collaborator

Why? This is V. Add a trailing struct param with a bool that tells the code whether or not to create the dir for you.

You could add that right away, with the default true for that bool, then tell people if they want the other behavior, pass false.

That is the least breaking way to handle it, while still allowing for the other behavior for those that want it.

@gechandesu

Copy link
Copy Markdown
Contributor Author

If we want to create a directory, even if it's optional, we need to change the function signature. It would be better to return Result to avoid panicking (which can't be handled now) and also to avoid hiding the error. Therefore, I prefer to remove the directory creation entirely, leaving only the path return, as is common.

Another way is to add new functions, but it can be difficult to find good names for them and this will also clutter the public API.

@spytheman

Copy link
Copy Markdown
Contributor

I do not see the value of not creating the folders.
Can you please describe the situation, where that is a problem?

Having a panic at the end, is not enough for me, and I would like to see a concrete example, where that was a problem for you.

@gechandesu

Copy link
Copy Markdown
Contributor Author

I don't have a real-world example where this would cause problems. If you're looking for these paths, you probably want to write or read something there, so it all seems logical.

I also looked closely at the possible errors that would trigger a panic. The list isn't too long, and they're either strange (for example, not having read or write permissions to your home directory), unrealistic (a path longer than 4 KB), or extreme cases where normal application operation is unlikely. These cases are probably truly worthy of a panic, but that doesn't change the fact that the panic is occurring in an unexpected place. I believe an API should provide the ability to handle any errors instead of panicking unconditionally where the library author deems it appropriate.

My motivation for this PR is to rid the API of actions that might be unexpected. As I wrote in the issue, it's usually expected to simply retrieve a path, not immediately create it.

If you're not convinced, please close the PR; I'll add documentation for these functions in a new PR.

@spytheman

Copy link
Copy Markdown
Contributor

@gechandesu thank you 🙇🏻 .

The intended use of the affected APIs, is to return paths, that are ready to be used, without further preparations/decisions, saving each app from having to re-implement some non trivial logic at each callsite.

I think documenting it (and that they can panic, if the corresponding folders do not exist, and could not be created) is a better path forward, from an ergonomic point of view (as well as not breaking backwards compatibility), so I will close this PR.

That said, I do think, that we can add a new set of APIs, that just return strings, formed by adding a _location suffix:
pub fn cache_dir_location() string {
pub fn data_dir_location() string {
pub fn local_bin_dir_location() string {
etc.

Those new APIs, can then work based on just string manipulation, without attempting to create or do any filesystem manipulations (that can panic), similar to how the python library you linked in the issue (when it is used without the ensure_exists property ... which btw suggests that others also found re-creating them, to be very useful).

I also noticed, that the rust library
https://github.com/xdg-rs/dirs/blob/master/dirs/src/lib.rs returns option values, presumably using none to indicate that the path did not exist, but that made little sense to me, since it loses information about the location, from the point of view of the caller 🤔 .

I am not worried about API clutter, since the new ones will use a common suffix, and because both the existing ones and the new ones, would be useful in different situations (brevity, vs wanting more explicit control over what is created/when), and because the compiler will just trim the ones, that are not used in any given application. The overhead would be just the normal parsing/checking time for a few more lines of code.

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.

3 participants