-
-
Notifications
You must be signed in to change notification settings - Fork 241
Use XDG_CONFIG_HOME if available #369
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
base: master
Are you sure you want to change the base?
Conversation
|
Great addition, I would also like something like this. Welcome to the world of GitHub submissions! It is a learning process for all of us, although be warned that some people forget that and get quite mean sometimes. I'm not a maintainer, but I'll leave a review momentarily to get more activity on this PR. Since this is your first PR and you seem to be unsure on etiquette, I'll mention a few things that hopefully help! Just disregard if you already know this:
|
zachcran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left feedback on some issues I saw and some potential options to discuss.
nb
Outdated
| # Check if XDG_CONFIG_HOME exists | ||
| if [[ -n "${XDG_CONFIG_HOME}" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I think this would always use XDG_CONFIG_HOME/nb/.nbrc if XDG_CONFIG_HOME is defined. It should probably still default to using $HOME/.nbrc if the file already exists so it doesn't break existing installations and setups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think you're right. I hadn't considered that.
nb
Outdated
| if [[ -n "${XDG_CONFIG_HOME}" ]]; then | ||
| # If XDG_CONFIG_HOME exists put .nbrc config file in a subdirectory. | ||
| mkdir -p "${XDG_CONFIG_HOME}/${_ME%%.*}" && | ||
| export NBRC_PATH="${NBRC_PATH:-"${XDG_CONFIG_HOME}/${_ME%%.*}/.${_ME}rc"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have commonly seen the main config file for programs installed under $XDG_CONFIG_DIR/subdir named config instead of something specific to the program like .nbrc. It may be worth considering here.
i.e.
| export NBRC_PATH="${NBRC_PATH:-"${XDG_CONFIG_HOME}/${_ME%%.*}/.${_ME}rc"}" | |
| export NBRC_PATH="${NBRC_PATH:-"${XDG_CONFIG_HOME}/${_ME%%.*}/config"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. But I think there's value in knowing that weather its in a notebook, or in XDG_CONFIG_HOME, that the file is (mostly) the same name. I did get rid of the . though.
nb
Outdated
| export NBRC_PATH="${NBRC_PATH:-"${XDG_CONFIG_HOME}/${_ME%%.*}/.${_ME}rc"}" | ||
| else | ||
| # Otherwise, use the default path | ||
| export NBRC_PATH="${NBRC_PATH:-"${HOME}/.${_ME}rc"}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option for the default config location is $HOME/.config/nb/ (the default value recommended in the XDG spec for XDG_CONFIG_HOME). Any thoughts from the maintainers/other users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if it made sense to conform to the spec, if XDG_CONFIG_HOME wasn't explicitly set, especially since you can have independent notebooks in folders withe their own .nbrc and that wasn't the current default. But I would prefer if nb followed the spec by default (Hence the patch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware that each notebook could have its own .nbrc file! I'm struggling to find where this mentioned in the user guide website (https://xwmx.github.io/nb/), but I think I see the behavior defined starting on L564. Do you know if this behavior is documented in the user guide website yet?
Regardless, as far as I can tell the default for NBRC_PATH does not affect sourcing notebook-specific .nbrc files. The notebook-specific settings are hard-coded to be named .nbrc and NB_NOTEBOOK_PATH seems to depend on NB_DIR but not NBRC_PATH. NBRC_PATH is sourced on L455 prior to $NB_NOTEBOOK_PATH/.nbrc on L567, so I think regardless of default location they will both be sourced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zachcran It's currently undocumented. If I recall correctly, the original use case where this came up was about using a specific editor for a notebook, which made me think about it as being more for user-specific settings, and therefore something that shouldn't be checked into the notebook repo. However, this would have to be .gitignored manually, so it seemed complicated and I decided to figure it out later. I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, that is correct that the notebook .nbrc should be unaffected by $NBRC_PATH. The settings in notebook .nbrc files should override any matching global settings regardless of where they are set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Sorry for possible duplicate notifications, GitHub was behaving a bit strangely there) Thanks for the explanation! Is the ability to have notebook-specific .nbrc files something that you would like documented if someone is willing to do it? I could make an issue about it and label it with something like "Help Wanted" for now if you would like (and maybe work on it in the future, no promises though).
therefore something that shouldn't be checked into the notebook repo. However, this would have to be .gitignored manually, so it seemed complicated and I decided to figure it out later. I'm open to suggestions.
Would excluding the .nbrc file during the git add call be sufficient? After some digging, I learned that you can use "exclude" pathspecs with git add --all to ignore certain files. This would just require changing git -C "${_notebook_path}" add --all on L3768 to git -C "${_notebook_path}" add --all ':!.nbrc'. We can create a separate issue to discuss this further if needed, since it is a bit off topic here.
Co-authored-by: Zachery Crandall <[email protected]>
Co-authored-by: Zachery Crandall <[email protected]>
|
Thanks for exploring this. I've thought about this a few times and haven't tackled it yet because it gets complicated. We should have thorough test coverage to make sure everything works. The |
Does |
zachcran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrDowntempo I took another look through this, and had some further suggestions as I am learning more about how the inner nb code works.
nb
Outdated
| # Check if XDG_CONFIG_HOME exists | ||
| if [[ -n "${XDG_CONFIG_HOME}" ]]; then | ||
| # If XDG_CONFIG_HOME exists put .nbrc config file in a subdirectory. | ||
| mkdir -p "${XDG_CONFIG_HOME}/${_ME%%.*}" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an _init_create_rc_file function defined here already. Should creation of the config directory be part of that function instead? I think it would just require one line before writing the default nbrc file, something like this:
mkdir -p "$(dirname ${NBRC_PATH})"Co-authored-by: Zachery Crandall <[email protected]>
Be warned. This is my first pull request EVAR. I'm not much of a coder, and I don't write my own bash scripts. I've tested this and it seeeems to work as a clean install, but I doubt it'll handle existing installs elegantly, if at all. Also this script is huuuge so I could've easily overlooked another place that needs an edit. If I've not gone about this the correct way, lemme know I'll try my best to do better next time.
This change should check if XDG_CONFIG_HOME exists, and if it does, create an nb subdirectoy in there for your .nbrc file (by way of the _ME variable)
I find having .nbrc in my home directory icky and want to tuck it away where it belongs. If this works, I'll try making XDG_STATE_HOME the default parent of the default notebook directory next.
This doesn't handle anything fancy like the preference-ordered XDG_CONFIG_DIRS variable.