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

refactor: Set fish_config with $__fish_config_dir #32

Merged
merged 1 commit into from
Mar 5, 2021
Merged

refactor: Set fish_config with $__fish_config_dir #32

merged 1 commit into from
Mar 5, 2021

Conversation

SirWrexes
Copy link
Contributor

Hello,
Is this to your liking ?
Have a good day. 👋

Copy link
Owner

@joseluisq joseluisq left a comment

Choose a reason for hiding this comment

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

Everything is fine. The only minor change that you could do taking advantage of your PR is this one on line 15:

-set -q GITNOW_CONFIG_FILE; or set -g GITNOW_CONFIG_FILE ~/.gitnow
+set -q GITNOW_CONFIG_FILE; or set -g GITNOW_CONFIG_FILE "$HOME/.gitnow"

@joseluisq joseluisq self-assigned this Mar 5, 2021
@joseluisq joseluisq added the enhancement New feature or request label Mar 5, 2021
@SirWrexes
Copy link
Contributor Author

SirWrexes commented Mar 5, 2021

Well I know it's a very rare occurrence but I did happen to have some programs not recognise '~' once or twice, whereas expanding an environment variable should always work and I don't really see how $HOME could be missing from the environment when spawning a shell.
I guess it's mostly a preference thing in the end though.
Shall I do the change ?

@joseluisq
Copy link
Owner

Well I know it's a very rare occurrence but I did happen to have some programs not recognise '~' once or twice, whereas expanding an environment variable should always work and I don't really see how $HOME could be missing from the environment when spawning a shell.

$HOME env is part of POSIX specification which means it must be present on system.

I guess it's mostly a preference thing in the end though.

Yes, just concordance with your PR. But yeah in practice both are essentially the same.

Shall I do the change ?

It is just a suggestion for "convention only" since your are introducing a $HOME too.
BTW this is the only place where ~ is used for paths.

@SirWrexes
Copy link
Contributor Author

Alrighty then !

@joseluisq joseluisq merged commit fbd57c0 into joseluisq:master Mar 5, 2021
@joseluisq
Copy link
Owner

Thanks!

@joseluisq
Copy link
Owner

Published on release 2.8.0

@SirWrexes SirWrexes deleted the fish-var branch March 7, 2021 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants