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

Follow XDG directories spec #251

Merged
merged 6 commits into from
Apr 21, 2022
Merged

Follow XDG directories spec #251

merged 6 commits into from
Apr 21, 2022

Conversation

cd-work
Copy link
Contributor

@cd-work cd-work commented Apr 12, 2022

Previously phylum's installer would put all files into ~/.phylum and
retrieve the configuration file from there. This leads to a rather
cluttered home directory which is why the XDG directories spec is
usually preferred.

The CLI will now check the XDG config directory first
(${XDG_CONFIG_HOME:-${HOME}/.config}/phylum/settings.yaml), then fall
back to the old config path if that isn't present.

The installer will put the completions in the XDG data directory
(${XDG_DATA_HOME:-${HOME}/.local/share}/phylum) and the binary in
${HOME}/.local/bin/.

Since the ${HOME}/.local/bin path is sometimes included in the path
already, without being present in the shell config itself, we check the
current $PATH first to try and detect if we need to add it or not.

Closes #224.

@cd-work cd-work requested a review from a team April 12, 2022 16:04
@cd-work cd-work requested a review from a team as a code owner April 12, 2022 16:04
Copy link
Contributor Author

@cd-work cd-work left a comment

Choose a reason for hiding this comment

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

I'd be curious how macOS users feel about ~/.local/share/phylum/completions and ~/.local/bin/phylum`. For Linux that isn't unexpected but macOS users might be confused?

cli/src/install.sh Outdated Show resolved Hide resolved
cli/src/install.sh Outdated Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
cli/src/install.sh Show resolved Hide resolved
@cd-work
Copy link
Contributor Author

cd-work commented Apr 13, 2022

I think it would likely make sense to put this PR on ice until we've resolved how we want to make things like completions/path modification optional.

It likely makes sense to follow rustup's example and put everything in a separate file where removal and changes are trivial. So it's probably better to change things once rather than having to figure out how to get to that new format from two possibly different starting points.

@louislang
Copy link
Contributor

I think it would likely make sense to put this PR on ice until we've resolved how we want to make things like completions/path modification optional.

I think this makes sense. Should we mark this as a draft so it doesn't accidentally get merged by someone?

@cd-work
Copy link
Contributor Author

cd-work commented Apr 19, 2022

I think this makes sense. Should we mark this as a draft so it doesn't accidentally get merged by someone?

I don't actually think it's possible to convert existing PRs to drafts? Unless you're suggesting to close and re-open as draft.

@cd-work
Copy link
Contributor Author

cd-work commented Apr 21, 2022

Decided to move things into a separate file in this PR, I think that's the easiest choice to avoid having to move things around multiple times.

@louislang
Copy link
Contributor

I don't actually think it's possible to convert existing PRs to drafts? Unless you're suggesting to close and re-open as draft.

You should be able to, just click the Still in progress? Convert to draft under reviewers in the top right!

Previously phylum's installer would put all files into `~/.phylum` and
retrieve the configuration file from there. This leads to a rather
cluttered home directory which is why the XDG directories spec is
usually preferred.

The CLI will now check the XDG config directory first
(`${XDG_CONFIG_HOME:-${HOME}/.config}/phylum/settings.yaml`), then fall
back to the old config path if that isn't present.

The installer will put the completions in the XDG data directory
(`${XDG_DATA_HOME:-${HOME}/.local/share}/phylum`) and the binary in
`${HOME}/.local/bin/`.

Since the `${HOME}/.local/bin` path is sometimes included in the path
already, without being present in the shell config itself, we check the
current `$PATH` first to try and detect if we need to add it or not.

Closes #224.
@samtay
Copy link
Contributor

samtay commented Apr 21, 2022

Worth mentioning that technically Apple specifies using $HOME/Library/Application Support for config/cache dirs like this. For example if you use the directories crate, you can see in their example that this is what they use for a project dir on Mac.

Personally I think this is unexpected for a CLI tool, so I'd vote to keep it under ~/.config/.

However it does raise the question, what is our story for Windows here? Looking at other apps (e.g. neovim, vs code), it seems many use %APPDATA%, but I can't tell if there's a particular standard. Not a Windows user myself.

@cd-work
Copy link
Contributor Author

cd-work commented Apr 21, 2022

Personally I think this is unexpected for a CLI tool, so I'd vote to keep it under ~/.config/.

Yes I agree, I've considered the dirs crate and decided to go this route instead. I would say for data dirs it might be a bit less clear but for config stuff ~/.config should definitely be the right place.

However it does raise the question, what is our story for Windows here? Looking at other apps (e.g. neovim, vs code), it seems many use %APPDATA%, but I can't tell if there's a particular standard. Not a Windows user myself.

It seems like right now Windows support only exists through Linux shims anyway. But I would recommend %APPDATA% should that ever change.

kylewillmon
kylewillmon previously approved these changes Apr 21, 2022
cli/src/install.sh Outdated Show resolved Hide resolved
kylewillmon
kylewillmon previously approved these changes Apr 21, 2022
cli/src/config.rs Outdated Show resolved Hide resolved
cli/src/config.rs Outdated Show resolved Hide resolved
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.

Adhere to XDG Base Directory standard
4 participants