-
Notifications
You must be signed in to change notification settings - Fork 74
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
Use XDG standard by default #304
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: c4llv07e <[email protected]>
tg/config.py
Outdated
|
||
CONFIG_DIR = os.path.expanduser("~/.config/tg/") | ||
CONFIG_DIR = expand_path("$XDG_CONFIG_HOME/tg/") |
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.
This doesn't work if $XDG_CONFIG_HOME isn't 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.
I'm not really sure about this solution, but it works both with and without variables.
Signed-off-by: c4llv07e <[email protected]>
Co-authored-by: arza <[email protected]>
@@ -11,14 +11,21 @@ | |||
_darwin = "Darwin" | |||
_linux = "Linux" | |||
|
|||
def expand_path(path): | |||
return os.path.expandvars(os.path.expanduser(path)) |
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.
What’s the purpose of expandvars()
here? This seems like it might introduce unexpected behaviour in case any of the paths might have a "$
" in them (for whatever reason).
The $XDG_*
variables are already "expanded" by os.getenv
, so e.g. os.getenv("XDG_CONFIG_HOME", "~/.config")
following export XDG_CONFIG_HOME=$HOME/.xdg_config
would return /home/user/.xdg_config
, not $XDG_CONFIG_HOME/.xdg_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.
If you are using default paths, then you're right. But what if someone changes cache folder location to "$XDG_CACHE_HOME/$USER/tg"? Then they couldn't expand it with os.getenv
.
If you use the default paths, then you are right. But what if someone changes the location of the cache folder to "$XDG_CACHE_HOME/$USER/tg"? Then they won't be able to expand it with `os.getenv'.
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.
You mean change the location inside tg
? Or set XDG_CACHE_HOME
? If the former, then this PR is introducing a feature to the codebase beyond what it says it’s set out to do and unrelated to the XDG standard. If the latter, then $USER
would be expanded by the system and stored in the XDG_CACHE_HOME
envvar directly, so there’d be no need to expand it again inside tg
.
And again, this may break in the case of a user for some reason or other decided to use a literal $
in their paths and it happens to resolve to a variable in the environment at time of evaluation.
It would be nice if tg followed XDG base directory specification.