RFC: deduplicate implementations for get_config_dir and get_cache_dir#17188
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
503c1d0 to
baf2e24
Compare
get_config_dir and get_cache_dir
| if cls._temp_path is not None: | ||
| xch = cls._temp_path | ||
| path = xch / rootname | ||
| if not path.is_file(): |
There was a problem hiding this comment.
note that this condition didn't exist in get_config_dir, but this implementation seems to pass all tests (while unconditionally calling path.mkdir doesn't), so I figured it's preferable to special casing cache.
659512d to
e0ab985
Compare
astropy/config/paths.py
Outdated
|
|
||
| def _get_dir_path(rootname: str, dirtype: _DirectoryType) -> Path: | ||
| match dirtype: | ||
| case _DirectoryType.CONFIG: |
There was a problem hiding this comment.
Seems a little more complicated than what I had in mind. It is very unlikely we will ever expand this, so I don't think we need to make it so flexible.
There was a problem hiding this comment.
I don't mean it to become more flexible, though is it a bad thing if it merely emerges as such ? this match/case is just a fancy if/elif/else, and the reason I'm combining it with an enum is that it makes the dispatching logic checkable by a type-checker (see #16603). Admittedly it's not required, but should I throw it away after the work is done ?
There was a problem hiding this comment.
Maybe I'll let @nstarman have a look then since he initially requested such refactoring and is very into typing stuff. Thanks for the clarification!
There was a problem hiding this comment.
I'm a bit with @pllim here, and would just have the call signature be _get_dir_path(fallback, cls, rootname="astropy") so that one can avoid the whole matching/ifelsing (fine to omit error checking since it is just used internally).
There was a problem hiding this comment.
I agree that the Enum + match is cool, but unnecessary in this case. An if-else would be best here unless we plan to add more options to the enum. The flag can be a bool, which is equally good type-wise for 2 options.
As soon as there's a 3rd option we should use the Enum (or another Literal-compatible flag)!
There was a problem hiding this comment.
Alright, by popular demand, here's a simpler impl (enum and match/case-free)
astropy/config/paths.py
Outdated
| raise ValueError(f"Unexpected {dirtype=}") | ||
|
|
||
| # If using set_temp_x, that overrides all | ||
| if cls._temp_path is not None: |
There was a problem hiding this comment.
Absolute nit, but a good opportunity for the walrus: if (xch := cls._tmp_path) is not None: (and nice for symmetry with the below).
astropy/config/paths.py
Outdated
|
|
||
| def _get_dir_path(rootname: str, dirtype: _DirectoryType) -> Path: | ||
| match dirtype: | ||
| case _DirectoryType.CONFIG: |
There was a problem hiding this comment.
I'm a bit with @pllim here, and would just have the call signature be _get_dir_path(fallback, cls, rootname="astropy") so that one can avoid the whole matching/ifelsing (fine to omit error checking since it is just used internally).
659512d to
5a80ec2
Compare
5a80ec2 to
a52c00f
Compare
|
Thanks, all! |
|
|
||
| # first look for XDG_CACHE_HOME | ||
| if ( | ||
| (xdg_config_home := os.getenv("XDG_CONFIG_HOME")) is not None |
There was a problem hiding this comment.
it should have been XDG_CACHE_HOME here I think, so currently the code is using only XDG_CONFIG_HOME I think, not XDG_CACHE_HOME.
There was a problem hiding this comment.
Indeed. For posterity this is being addressed in #17514
Description
Follow up to #16997, as requested by @nstarman