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

Auto-expand tilde (home directory) for universal variable nvm_data #190

Closed
thernstig opened this issue Jul 4, 2022 · 12 comments
Closed
Labels
enhancement New feature or fix

Comments

@thernstig
Copy link

This is the same issue as fixed here jorgebucaran/fisher#611

Currently nvm_data is set to something like nvm_data:/home/userA/\x2elocal/share/nvm which makes it harder to sync the fish_variables file. This could be solved the same way as was done for fisher, which would it possible/easier to sync the dotfiles.

(Since you are the author for both @jorgebucaran you know what this is about, hence why I am not more specific)

@jorgebucaran
Copy link
Owner

Or perhaps just change the variable scope to global? 🤓

@thernstig
Copy link
Author

If that works ok, sure :)

@debashisbiswas
Copy link

debashisbiswas commented Jan 30, 2023

Following the above recommendation, I changed nvm_data from universal to global, moving it out of fish_variables and into conf.d/nvm.fish (not config.fish, which happens after the default version is loaded), which worked for my use case.

Is this something you'd be interested in this project doing? If so, I wouldn't mind making the PR.

@jorgebucaran
Copy link
Owner

nvm_data is universal because 2f2af43. If we can find a way to change it back to global without creating new issues, that'd be great.

@jorgebucaran jorgebucaran added the enhancement New feature or fix label Jan 30, 2023
@rhenning
Copy link

rhenning commented Feb 4, 2023

👋🏼 just stumbled across this while syncing config with chezmoi across a couple of machines, which inadvertently picked up the statically-rendered interpolation of nvm_data in fish_variables. i began to work around it with a template but then started to wonder how it got set... glad to see others are thinking about it too.

glad to help PR, test, buy coffee, etc. 😆

p.s. many thanks for your contributions to the community, @jorgebucaran. awesome work!

@jorgebucaran
Copy link
Owner

We could just revert the scope of nvm_data as per @debashisbiswas' suggestion.

See the commit above to see why we changed the scope to universal. Thank you, @rhenning! 😁

@lorthirk
Copy link

lorthirk commented Feb 4, 2023

👋🏼 just stumbled across this while syncing config with chezmoi across a couple of machines

The world is such a small place... I am in the very same situation! I've been watching this issue for a long time, and I am also available for anything!

@debashisbiswas
Copy link

nvm_data is universal because 2f2af43. If we can find a way to change it back to global without creating new issues, that'd be great.

I read through the issues that this commit was meant to fix. To provide context for this discussion: the reason it was made universal is to avoid requiring the user to restart their terminal after installing, and to avoid issues in which their config was set up to use nvm before nvm.fish was run.

@lorthirk
Copy link

lorthirk commented Apr 3, 2023

I would love to think of a way to fix this, but I'm afraid that the context @debashisbiswas provided is not enough for me to fully understand ths issue 😕 any other hint would be greatly appreciated

@debashisbiswas
Copy link

@lorthirk It's been a few months, so I've forgotten the specifics, but I can try to give you a bit more context. Looking at 2f2af43 and the related issues, there seem to be a few reasons why the variable was made universal. The issues in question are #130, #139, and #140.

Universal variables are applied immediately, and available to all sessions. On the other hand, global variables are per-session, and must be set before they are used within the session. Previously, the global variables were set in conf.d/nvm.fish, which runs when the session starts. Right after a fresh install, this file hasn't been run yet, so trying to use nvm immediately after installing resulted in errors. This means that users needed to start a new session after install. This caused some confusion for users, so universal variables were used instead. I believe this is what happened in #130 and #140. It should be possible to achieve the same behavior with global variables if they're managed correctly.

Also, config files in fish are loaded on startup in a specific order. If the user's config is set up to use nvm before conf.d/nvm.fish is run (for example, by using nvm install in conf.d/aaa.fish, which would run before), there will be issues with variables being uninitialized, which results in the same error as the previous paragraph. This is what happened in #139, and universal variables solve this as well. I am not sure that there's a way around this with global variables, but this case may not be necessary for this plugin to handle, as this is more specifically related to how a user's fish config is set up.

@lorthirk
Copy link

lorthirk commented Apr 4, 2023

Thanks @debashisbiswas. Now that you explained it in such an easy to understand way, I'm not so sure any fix we could come up with would be better than "hackish". I have a naive question: would be that bad to add a warning message when installing nvm.fish that warns about such issues? Honestly, restarting a session is not that bad once you know you have to do, and in general I wouldn't let this drive the architecture of an application. As per the other issue, as you said, this really sounds like something a user should be warned of and arrange his/her custom configuration accordingly.

What do you guys think?

@jorgebucaran
Copy link
Owner

Fixed. Thanks, everyone! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or fix
Projects
None yet
Development

No branches or pull requests

5 participants