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.

Sign up for GitHub

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

Jump to bottom

Fix: XDG_CONFIG_HOME compatible #1888

Merged
merged 1 commit into from
Sep 10, 2018
Merged

Fix: XDG_CONFIG_HOME compatible #1888

merged 1 commit into from
Sep 10, 2018

Conversation

Lilja
Copy link
Contributor

@Lilja Lilja commented Aug 14, 2018 •
edited by ljharb
Loading

Fixes #1088, fixes #1882.

I haven't really tried it, so would be good if someone tried it at least.

@Lilja Lilja mentioned this pull request Aug 14, 2018
Closed
ljharb
ljharb requested changes Aug 15, 2018
View reviewed changes
README.md Outdated
@@ -52,10 +52,10 @@ or Wget:
wget -qO- https://raw.githubusercontent.com/creationix/nvm/v0.33.11/install.sh | bash
```

<sub>The script clones the nvm repository to `~/.nvm` and adds the source line to your profile (`~/.bash_profile`, `~/.zshrc`, `~/.profile`, or `~/.bashrc`).</sub>
<sub>The script clones the nvm repository to `$XDG_CONFIG_HOME/nvm` and adds the source line to your profile (`~/.bash_profile`, `~/.zshrc`, `~/.profile`, or `~/.bashrc`).
Copy link
Member

Choose a reason for hiding this comment

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

I'd still leave this as ~/.nvm, but maybe clarify that it will go inside $XDG_CONFIG_HOME if present, otherwise will fall back to $HOME.

(also, you've lost the </sub> here)

README.md Outdated

```sh
export NVM_DIR="$HOME/.nvm"
export NVM_DIR="${XDG_CONFIG_HOME:-$HOME/config}/nvm"
Copy link
Member

Choose a reason for hiding this comment

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

this seems like it'd put it in either $XDG_CONFIG_HOME/nvm, or $HOME/config/nvm - however, the locations i'd expect are $XDG_CONFIG_HOME/.nvm or $HOME/.nvm.

install.sh Outdated
elif [ ! -z "$XDG_CONFIG_HOME" ]; then
printf %s "${XDG_CONFIG_HOME/nvm/}"
else
printf %s "${HOME/.config/nvm/}"
Copy link
Member

Choose a reason for hiding this comment

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

inside the home dir, it must be .nvm directly inside $HOME - there's no .config directory convention i'm interested in or aware of.

ljharb
ljharb requested changes Aug 15, 2018
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I'm still not clear why the folder name should be "nvm" and not ".nvm" inside the XDG config folder. Can you explain?

README.md Outdated
@@ -52,10 +52,12 @@ or Wget:
wget -qO- https://raw.githubusercontent.com/creationix/nvm/v0.33.11/install.sh | bash
```

<sub>The script clones the nvm repository to `~/.nvm` and adds the source line to your profile (`~/.bash_profile`, `~/.zshrc`, `~/.profile`, or `~/.bashrc`).</sub>
<sub>The script clones the nvm repository to `$HOME/nvm` and adds the source line to your profile (`~/.bash_profile`, `~/.zshrc`, `~/.profile`, or `~/.bashrc`).</sub>
Copy link
Member

Choose a reason for hiding this comment

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

.nvm not nvm

nvm.sh Outdated
nvm_echo "${legacy_nvm_dir}/.nvmrc"
fi
if [ -e "${XDG_CONFIG_HOME}/nvm/.nvmrc" ]; then
nvm_echo "${XDG_CONFIG_HOME}/nvm/.nvmrc"
Copy link
Member

Choose a reason for hiding this comment

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

this will print out two lines if both locations exist.

I'm not actually sure why this is being changed; .nvmrc doesn't work currently inside $NVM_DIR when it's $HOME/.nvm; i'm not sure why it'd start working inside the XDG folder. If you want a global default, you'd do nvm alias default whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps then the way forward is figuring out why .nvmrc doesn't work inside the $NVM_DIR?

Copy link
Member

Choose a reason for hiding this comment

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

Because it's not supposed to work inside there. It's an rc file - it goes in the directory hierarchy, and $NVM_DIR isn't part of the hierarchy.

@Lilja
Copy link
Contributor Author

Lilja commented Aug 15, 2018

I'm still not clear why the folder name should be "nvm" and not ".nvm" inside the XDG config folder. Can you explain?

Going by what i'm currently seeing my own $XDG_CONFIG_HOME, this is it:

(venv) lilja in ~ on master % ls -al .config/
total 0
drwx------   6 lilja  lilja   192 Nov 21  2017 .
drwxr-xr-x+ 64 lilja  lilja  2048 Aug 15 09:36 ..
drwx------   6 lilja  lilja   192 Aug 13 16:35 configstore
drwx------   3 lilja  lilja    96 Mar  8 13:48 gtk-2.0
drwxr-xr-x   6 lilja  lilja   192 Dec 27  2017 timelogs
drwxr-xr-x   4 lilja  lilja   128 Nov  6  2017 yarn

Granted I don't have a lot of folders here, but there doesn't seem to be any folders that keep the dot-prefix before the folder names here.

@ljharb
Copy link
Member

ljharb commented Aug 15, 2018

Interesting; I'm not super familiar with XDG but if that's the convention then that's fine.

@Lilja
Copy link
Contributor Author

Lilja commented Aug 15, 2018

Don't understand why the other test cases are failing. Could anyone help me out?

@ljharb
Copy link
Member

ljharb commented Aug 15, 2018

I've got a fix in your PR #1891, thanks. Will merge once that's passed.

@Lilja
Copy link
Contributor Author

Lilja commented Aug 16, 2018

Anything else you need for a merge?

ljharb
ljharb approved these changes Aug 17, 2018
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM! just one question.

README.md Show resolved Hide resolved
@ljharb ljharb added the feature requests I want a new feature in nvm! label Aug 17, 2018
@ljharb ljharb merged commit 8542df4 into nvm-sh:master Sep 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
feature requests I want a new feature in nvm!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Request: Use $XDG_CACHE_HOME when defined unclutter $HOME and make nvm XDG compatible
4 participants
@Lilja @ljharb @turnerguo @douglasnaphas