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

comply with XDG Base Directory Specification (fixes #13) #14

Merged
merged 1 commit into from Sep 3, 2013

Conversation

aspiers
Copy link

@aspiers aspiers commented Apr 30, 2013

User config tree now defaults to ~/.config/rapport

I'm not 100% sure this works yet, because I still have a test failure ...

@aspiers
Copy link
Author

aspiers commented May 1, 2013

Hmm, according to XDG, created reports and other user data which isn't config should actually go in ~/.local/share/rapport, or whatever $XDG_DATA_HOME is set to. So don't merge this yet.

@aspiers
Copy link
Author

aspiers commented May 1, 2013

OK, I think this is good to merge now.

@@ -0,0 +1,27 @@
# Copyright (c) 2013, Sascha Peilicke <saschpe@gmx.de>
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't you have to add your name here? ;-)

Copy link
Author

Choose a reason for hiding this comment

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

I suppose I could ;-)

@saschpe
Copy link
Owner

saschpe commented May 13, 2013

Have you looked at the Python XDG bindings, i.e. http://pyxdg.readthedocs.org/en/latest/index.html ? I think it's worth using it for complying to the Base Directory spec.

@aspiers
Copy link
Author

aspiers commented May 13, 2013

Drat, you're right (if you're OK with the extra dependency, that is). Maybe worth merging this as an intermediate step and then switching it to use PyXDG later? Probably easier than reworking this PR.

@saschpe
Copy link
Owner

saschpe commented May 13, 2013

Sure, makes sense to me. Generally, I'm fine with extra dependencies since it's less code to take care here. With regards to openSUSE, we're in the comfortable situation that we have all those deps packaged.

I only try to avoid dependencies if they bring many transitive dependencies or if they add little gain. For instance, most Github bindings (applies to any programming language) are so over-engineered that a simple GET request to a JSON route is usually my preferred way to go. When programming in Python, I tend to pick whatever makes the code simpler and more obvious.

As an example, once I figured out what I can get from Launchpad via plain HTTP, I probably want to drop launchpadlib since it's really ugly and depends on countless other (bad) Python packages.

/etc/rapport/
"""
config_dirs = [
os.path.expanduser(os.path.join("~", ".rapport")),
USER_CONFIG_DIR,
Copy link
Owner

Choose a reason for hiding this comment

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

In order to have a migration path for existing users, I'd rather just add the USER_CONFIG_DIR line above and not drop the old config location. Alternatively, we should move the old config file to the new location.

@aspiers
Copy link
Author

aspiers commented Jul 29, 2013

Rebased onto latest upstream master. Remaining issues not addressed yet.

User config tree now defaults to ~/.config/rapport, and
reports dir to ~/.local/share/rapport/reports.  Added a
new unit test for the latter.

http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
@aspiers
Copy link
Author

aspiers commented Sep 2, 2013

Rebased again, and ditto.

saschpe added a commit that referenced this pull request Sep 3, 2013
Let's have it merged then. I'll add another commit to keep the old config locations with lower priority. If necessary, migration code can still be written later on
@saschpe saschpe merged commit 15ba02a into saschpe:master Sep 3, 2013
@aspiers aspiers deleted the xdg-compliance branch September 3, 2013 08:30
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.

None yet

2 participants