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

Change order of list returned by config_dirs() #168

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

noiioiu
Copy link

@noiioiu noiioiu commented Nov 17, 2024

This should fix a bug where beet uses ~/.config instead of XDG_CONFIG_HOME.

@noiioiu
Copy link
Author

noiioiu commented Nov 17, 2024

Actually, I think the correct behaviour (per the specification) is to use '~/.config' only if XDG_CONFIG_HOME is unset or empty, and to ignore all paths in XDG_CONFIG_HOME and XDG_CONFIG_DIRS that are not absolute. (This means that, if XDG_CONFIG_DIRS is a relative path, then only the directories in XDG_CONFIG_DIRS are used.) I've edited util.py to reflect this.

Copy link

@bbenne10 bbenne10 left a comment

Choose a reason for hiding this comment

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

Coming from the beets issue you opened (but not the one I'm searching for) and I wanted to particularly point out the issue with XDG behavior on MacOS. The rest is 100% pedantic. Hope this helps - if not, feel free to ignore me, a random guy who has never before contributed to the confuse code...

and UNIX_DIR_FALLBACK otherwise
"""
config_path = os.getenv('XDG_CONFIG_HOME', UNIX_DIR_FALLBACK)
return config_path if config_path != '' else UNIX_DIR_FALLBACK

Choose a reason for hiding this comment

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

Empty string is falsy. Use as a boolean.

Suggested change
return config_path if config_path != '' else UNIX_DIR_FALLBACK
return config_path or UNIX_DIR_FALLBACK

if 'XDG_CONFIG_HOME' in os.environ:
paths.append(os.environ['XDG_CONFIG_HOME'])
if 'XDG_CONFIG_DIRS' in os.environ:
if 'XDG_CONFIG_DIRS' in os.environ and os.environ['XDG_CONFIG_DIRS'] != '':

Choose a reason for hiding this comment

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

No need to look before leap here, just use the default fallback behavior (will return None if not present and empty string is falsy):

Suggested change
if 'XDG_CONFIG_DIRS' in os.environ and os.environ['XDG_CONFIG_DIRS'] != '':
if os.environ.get('XDG_CONFIG_DIRS'):

@@ -155,7 +161,7 @@ def config_dirs():
paths = []

if platform.system() == 'Darwin':
paths.append(UNIX_DIR_FALLBACK)
paths.append(xdg_config_home())

Choose a reason for hiding this comment

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

XDG does not apply to Darwin. These files should end up somewhere in ~/Library on that platform.

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

Successfully merging this pull request may close these issues.

2 participants