-
Notifications
You must be signed in to change notification settings - Fork 280
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
Make config related paths dynamical. Fixes #3104 #3105
Conversation
7255222
to
49bfe83
Compare
Not sure if it helps, but pooch (an optional dependency for yt) depends on appdirs which abstracts this a bit. |
953ee21
to
0fa4b90
Compare
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.
Thank you for doing this; I left a couple notes but overall LGTM
from yt.testing import assert_raises, fake_random_ds | ||
|
||
XDG_CONFIG_HOME = os.environ.get("XDG_CONFIG_HOME") |
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.
why isn't this part of the test setup ?
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 difference? It's not going to be imported by anything
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're right, but in this state it looks like it's going to be used in more than one test (which would make it a candidate for fixture in pytest), but it's not the case, so might as well define it in the only scope it used.
a2991a6
to
2c09502
Compare
Sanitize conf/plugin handling in tests
2c09502
to
b230cc5
Compare
PR Summary
This PR ensures that every time a "config path" is requested it respects the current value of XDG_HOME_DIR.