Use XDG Base Directory variables when creating the base installation directory - #1081
Conversation
lstocchi
left a comment
lstocchi
left a comment
There was a problem hiding this comment.
I didn't know the XDG base directory specification so thanks for this. Very interesting.
The only concern I have is to use a different default path when the user is not adhering to this specification. See comment below.
The other aspect is we need to update the README otherwise nobody would know that the extension leverages it.
BTW I'm also open at accepting all changes, I would wait to hear from @itowlson
|
|
||
| let xdgStateHome = process.env.XDG_STATE_HOME || ``; | ||
| if (xdgStateHome[0] !== `/`) { | ||
| xdgStateHome = path.join(os.homedir(), `.local/state`); |
There was a problem hiding this comment.
I think it would be better to keep the default path as it is now path.join(shell.home(), '.vs-kubernetes'). If one deletes the existing folder for some reason and then it is recreated somewhere else the user doesn't get what's going on.
Then by doing it we would only have two different cases 1) user who doesn't use XDG specification 2) users who use it... and we don't need to remember that old linux/windows/mac users uses .vs-kubernetes folder, new linux users use .local/state and people who use xdg specification may have their custom folder.
There was a problem hiding this comment.
Ah, so if I understand correctly, your point is that it would be less confusing to users if there is an explicit "opt-in" to the "XDG Base Dir Specification". So for example, we would only follow the spec if XDG_STATE_HOME is set explicitly?
I feel like doing it this way would be more counterintuitive for the user and we would realize less gains from attempting to use the spec in the first place. Mostly because most users expect things to go in ~/.local/state, ~/.config, etc. automatically. The spirit of the spec is that this should be automatic behavior, and if we have to check if XDG_STATE_HOME is set (which very few people actually do), then most people wouldn't be affected by this benefit (and in this case i would rather the PR not be merged since its an incomplete solution). There is also the issue of potentially having to do more of these checks for every new XDG variable used (assuming consistency is valued here) - whereas currently we only ever need to do existsSync(~/.vs-kubernetes) once
I think your statement about the end-user confusion when deleting ~/.vs-kubernetes and it being recreated somewhere else is definitely valid, and I believe your mention of updating the README to address this PR in general can address that. Maybe we can say somewhere in the README along the lines of "the extension will follow the XDG Base Directory Specification appropriately (defaulting to writing config and other files in ~/.local/state/vs-kubernetes), but only if the legacy directory of ~/.vs-kubernetes does not exist". That way, if the user searches for "config" or ".vs-kubernetes" in the docs, they know what's up right away
There was a problem hiding this comment.
I like "use the legacy directory if it exists, otherwise use the XDG directory." Seems like a nice pragmatic solution.
|
@hyperupcall Could you sign off for the DCO please? Thanks! |
Signed-off-by: Edwin Kofler <edwin@kofler.dev>
Prevents user confusion since the directory might be re-createad in the newer location if the older location is removed Signed-off-by: Edwin Kofler <edwin@kofler.dev>
|
DCO done! 👍 Also as mentioned before, I also updated the readme to explicitly document this to prevent any user confusion :) |
3 participants
The XDG Base Directory Specification specifies various user directories for applications to store data. There are a lot of good reasons to support this, and this PR adds support in a backwards-compatible way
vs-kubernetesdirectory in~/.local/state, but only if a directory has not been created at~/.vs-kubernetesand the current platform is Linuxvs-kubernetesdirectory is created with theXDG_STATE_HOMEvariable. If the env var is empty or if it's a relative path (as per the spec), the default of~/.local/stateis used