RFC: rewrite `config.paths._get_dir_path` as a class method by neutrinoceros · Pull Request #17554 · astropy/astropy

neutrinoceros · 2024-12-18T07:47:56Z

Description

Follow up to #17188 and #17546
This was initially discussed in #17546 (comment). I'll quote myself here for convenience:

I think your new annotations are revealing a small design issue with these functions:
if fallback can only be exactly 'cache' or 'config', shouldn't this mean that cls can only be exactly set_temp_cache or set_temp_config ? We could also naturally exclude invalid combinations like cls=set_temp_cache, fallback='config' by simply removing the fallback argument and making it a class attribute instead (in which case we can also keep the generalized annotation you used for cls)

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-12-18T07:48:25Z

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.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

github-actions · 2024-12-18T07:48:28Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

eerovaher · 2024-12-18T14:20:02Z

astropy/config/paths.py

+    _, _, fallback = cls.__name__.rpartition("_")
    if (
        (xdg_dir := os.getenv(f"XDG_{fallback.upper()}_HOME")) is not None


fallback is used for constructing the name of an environment variable. Having a fallback parameter with the type Literal["cache", "config"] lets type checkers ensure that the environment variable that will be looked up is valid. The change here removes that information from the type system because the __name__ of a subclass of _SetTempPath is not restricted.

The current annotation is not perfect because it does not prevent calls like _get_dir_path(..., set_temp_cache, "config"), so it can be a good idea to introduce a class attribute for set_temp_cache and set_temp_config to eliminate such calling errors, but trying to extract something from the class name is not good enough.

I hear you. How about this ?

neutrinoceros · 2024-12-18T14:47:34Z

astropy/config/paths.py


        return wrapper

+    @classmethod


I'm intentionally avoiding astropy.utils.decorators.classproperty because this module is so low in the stack that I don't think it's wise to make it depend on other parts of astropy

eerovaher · 2024-12-18T15:42:15Z

I'm starting to get the impression that _get_dir_path() should be a class method in _SetTempPath, but I might not have time to think this through properly this week.

pllim

Given #17507 discussions, I am not sure if this is worth doing...

neutrinoceros · 2024-12-19T09:26:46Z

I'm not sure what you mean here @pllim. Are you worried that we paint ourselves in a corner with this PR ? IMO, removing existing complexity will likely help more than hurt with adding new functionality. But I'm actually neutral.

I'm starting to get the impression that _get_dir_path() should be a class method in _SetTempPath, but I might not have time to think this through properly this week.

This comment grew on me and it now seems clear to me that this function belongs in this class. I'm going to push this change now, hopefully you'll agree with it when you have more time.

pllim · 2024-12-19T15:05:01Z

I am not sure yet but I have a feeling that all this code will change when we support custom env var.

neutrinoceros · 2024-12-19T15:42:59Z

It will indeed, but I expect that change will be almost completely additive. I could open a follow up PR as a draft if it helps lifting your hesitation.

neutrinoceros · 2024-12-19T17:59:25Z

@pllim I've opened #17567 to address #17507

eerovaher · 2024-12-25T21:48:41Z

astropy/config/paths.py

@@ -145,6 +119,7 @@ def get_cache_dir(rootname: str = "astropy") -> str:
 class _SetTempPath:
    _temp_path: Path | None = None
    _default_path_getter: Callable[[str], str]


Do we need _default_path_getter at all anymore?

Not really. It can easily be refactored out as follow:

diff --git a/astropy/config/paths.py b/astropy/config/paths.py index 35ee8b2786..73e62ddb1b 100644 --- a/astropy/config/paths.py +++ b/astropy/config/paths.py @@ -118,7 +118,6 @@ if get_cache_dir_path.__doc__ is not None: class _SetTempPath: _temp_path: Path | None = None - _default_path_getter: Callable[[str], str] _fallback_dirname: Literal["cache", "config"] def __init__( @@ -133,8 +132,9 @@ class _SetTempPath: def __enter__(self) -> str: self.__class__._temp_path = self._path + mtd = get_cache_dir if self._fallback_dirname == "cache" else get_config_dir try: - return self._default_path_getter("astropy") + return mtd(rootname="astropy") except Exception: self.__class__._temp_path = self._prev_path raise @@ -211,7 +211,6 @@ class set_temp_config(_SetTempPath): context (default: False). """ - _default_path_getter = staticmethod(get_config_dir) _fallback_dirname = "config" def __enter__(self) -> str: @@ -267,7 +266,6 @@ class set_temp_cache(_SetTempPath): context (default: False). """ - _default_path_getter = staticmethod(get_cache_dir) _fallback_dirname = "cache"

and it would be a good idea to do something similar, as it currently only serves as a confusing layer of indirection (IMO).
However, I think it's out of scope here. Can I do it as a follow up PR ?

Can't you use cls.get_dir_path instead of mtd?

indeed, that's a lot cleaner !

eerovaher · 2024-12-25T21:49:06Z

astropy/config/paths.py

 class _SetTempPath:
    _temp_path: Path | None = None
    _default_path_getter: Callable[[str], str]
+    _fallback_dirname: Literal["cache", "config"]


There should be a comment to explain why there are only two allowed values.

eerovaher · 2024-12-30T19:42:06Z

astropy/config/paths.py

        return wrapper

+    @classmethod
+    def get_dir_path(cls, rootname: str) -> Path:


Why should this be a public method?

My reasoning is that this method being used outside the class itself, it shouldn't be private, though I understand there's an ambiguity here about how we interpret "private".

As Eero pointed out above, might be slightly less controversial to keep the leading underscore since the original name already has it. There is no expectation that this new class method will be used beyond this module, right?

There is no expectation that this new class method will be used beyond this module, right?

indeed, there isn't

eerovaher · 2024-12-30T19:43:56Z

astropy/config/paths.py

+        else:
+            linkto = None
+
+        return _find_or_create_root_dir(cls._fallback_dirname, linkto, rootname)


If _find_or_create_root_dir were a class method too then we wouldn't have to pass cls._fallback_dirname as an argument when we call it.

good point, let me give this a spin

pllim

Seems reasonable. Thanks!

pllim · 2025-01-07T20:20:08Z

@eerovaher , do you want to do a final review?

eerovaher · 2025-01-09T18:11:58Z

astropy/config/paths.py

+            return path.resolve()
+
+        if (
+            (xdg_dir := os.getenv(f"XDG_{cls._fallback_dirname.upper()}_HOME"))


Currently the code is storing the name of the corresponding directory on the class as _fallback_dirname (by the way, that's not a good name) and constructing the name of the environment variable from _fallback_dirname at runtime. It looks like this is trying to be too clever and it would be simpler to store both the directory name and the environment variable name in separate class attributes.

all fair points. I've taken this into account in my latest update.

eerovaher · 2025-01-10T21:34:00Z

astropy/config/paths.py

+    # This base class serves as a deduplication layer for its only two intended
+    # children (set_temp_cache and set_temp_config)
+    _directory_type: Literal["cache", "config"]
+    _directory_env_var: str


_directory_env_var should be annotated using Literal because it can only hold a small set of values which we are not free to choose because they are defined by an external standard. It is better to document that in the annotation than in a comment.

I'm not satisfied with this suggestion because it leaves room for 2 invalid cases, namely

_directory_typ = "cache" _directory_env_var = "XDG_CONFIG_HOME"

and

_directory_typ = "config" _directory_env_var = "XDG_CACHE_HOME"

So I refactored this again into an enum. Now, the pairing is expressed in the annotation.

update: my enum refactor was broken beyond repair. Welp, maybe I'll stop chasing perfection here.

I still think that it's simpler to check that these two attributes that are right next to each other agree when reading the code than it would be to understand some clever mechanism that would generate one from the other at runtime.

Agreed. I'm still slightly frustrated by the outcome but I can certainly live with it !

eerovaher

This pull request was triggered by the observation that the cls and fallback parameters of _get_dir_path() were not independent from each other, which suggested that the fallback parameter should instead be a class attribute in the _SetTempPath subclasses. Another observation was that because _get_dir_path() expected cls to be a subclass of _SetTempPath then it essentially was a class method of _SetTempPath even if it was not explicitly decorated as such. A few more similar changes followed from that.

Overall this pull request simplifies the code because it ensures that functions that are tightly coupled with _SetTempPath are defined as methods of that class instead of being scattered all over the module.

pllim · 2025-01-14T15:49:52Z

astropy/config/paths.py

+    # This base class serves as a deduplication layer for its only two intended
+    # children (set_temp_cache and set_temp_config)
+    _directory_type: Literal["cache", "config"]
+    _xdg_directory_env_var: Literal["XDG_CACHE_HOME", "XDG_CONFIG_HOME"]


Given the plan to follow up with #17567 for a different env var name, maybe no need to hardcode xdg into the new class attribute now?

pllim · 2025-01-14T18:54:24Z

Thanks for your patience!

neutrinoceros added this to the v7.1.0 milestone Dec 18, 2024

neutrinoceros added Refactoring no-changelog-entry-needed labels Dec 18, 2024

neutrinoceros requested a review from eerovaher December 18, 2024 07:47

neutrinoceros mentioned this pull request Dec 18, 2024

Add more type annotations to config #17546

Merged

1 task

github-actions bot added the config label Dec 18, 2024

neutrinoceros marked this pull request as ready for review December 18, 2024 07:59

eerovaher reviewed Dec 18, 2024

View reviewed changes

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch from 4f50fa9 to 4d77c3f Compare December 18, 2024 14:45

neutrinoceros commented Dec 18, 2024

View reviewed changes

neutrinoceros changed the title ~~RFC: simplify config.paths._get_dir_path~~ RFC: simplify config.paths._get_dir_path Dec 18, 2024

pllim reviewed Dec 18, 2024

View reviewed changes

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch from 4d77c3f to c716fb5 Compare December 19, 2024 09:27

neutrinoceros changed the title ~~RFC: simplify config.paths._get_dir_path~~ RFC: rewrite config.paths._get_dir_path as a class method Dec 19, 2024

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch from c716fb5 to 2ff0d45 Compare December 19, 2024 09:29

neutrinoceros mentioned this pull request Dec 19, 2024

ENH: add support for setting astropy's cache and config directories via environment variables #17567

Closed

4 tasks

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch from 2ff0d45 to c51d91c Compare December 20, 2024 07:33

eerovaher reviewed Dec 25, 2024

View reviewed changes

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch from c51d91c to 7b9fbb4 Compare December 26, 2024 10:26

eerovaher reviewed Dec 30, 2024

View reviewed changes

neutrinoceros marked this pull request as draft January 1, 2025 09:38

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch from 13a9bf0 to e6aa8af Compare January 1, 2025 09:59

neutrinoceros marked this pull request as ready for review January 1, 2025 10:04

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch 2 times, most recently from 6b9683e to 4170299 Compare January 7, 2025 16:51

pllim approved these changes Jan 7, 2025

View reviewed changes

eerovaher reviewed Jan 9, 2025

View reviewed changes

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch from 4170299 to b842871 Compare January 10, 2025 15:05

eerovaher reviewed Jan 10, 2025

View reviewed changes

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch 2 times, most recently from b05d4ea to 7531359 Compare January 11, 2025 08:32

eerovaher approved these changes Jan 13, 2025

View reviewed changes

pllim reviewed Jan 14, 2025

View reviewed changes

RFC: rewrite config.paths._get_dir_path as a class method

6956a5c

neutrinoceros force-pushed the config/rfc/simplify_get_dir_path branch from 7531359 to 6956a5c Compare January 14, 2025 16:14

pllim merged commit 8594e8e into astropy:main Jan 14, 2025
23 of 24 checks passed

neutrinoceros deleted the config/rfc/simplify_get_dir_path branch January 14, 2025 19:41

Uh oh!

Conversation

neutrinoceros commented Dec 18, 2024

Description

Uh oh!

github-actions bot commented Dec 18, 2024

Uh oh!

github-actions bot commented Dec 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Dec 18, 2024

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Dec 19, 2024

Uh oh!

pllim commented Dec 19, 2024

Uh oh!

neutrinoceros commented Dec 19, 2024

Uh oh!

neutrinoceros commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Jan 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerovaher Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Dec 18, 2024 •

edited

Loading

neutrinoceros commented Dec 19, 2024 •

edited

Loading

neutrinoceros Dec 26, 2024 •

edited

Loading

eerovaher Jan 10, 2025 •

edited

Loading

neutrinoceros Jan 11, 2025 •

edited

Loading