MNT: Run PTH flake test in prep for supporting pathlib (config) by neutrinoceros · Pull Request #16997 · astropy/astropy

neutrinoceros · 2024-09-11T13:05:13Z

Description

Ref #16924
This is in the continuation of #16060

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-09-11T13:05:43Z

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-09-11T13:05:46Z

👋 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?

neutrinoceros · 2024-09-11T13:09:22Z

astropy/config/paths.py

-        homedir = os.path.expanduser("~")
-    except Exception:
+        return Path.home()
+    except RuntimeError:


the content of this except block could be dedented but that would make the diff much harder to read.

It is very simple to ignore lines where the only change is in the indentation level, both with Git (git diff --ignore-space-change) and in the GitHub UI.

oh I didn't know GitHub supported that, good to know. Still, I'll only do it if requested, as I'd like these PRs to be small and quick to expedite.

eerovaher · 2024-09-11T15:07:11Z

astropy/config/paths.py



-def _find_home():
+def _find_home() -> Path:


It's not at all obvious to me why we need this function at all. Why couldn't we just use Path.home()? The vast majority of this function is for Windows, so we need @pllim to have a look at this.

yeah, codecov seems useful for once and shows that none of the except block is used in any test.

Blame says this code was added at least 13 years ago (#73), so probably predated a lot of Path stuff and maybe even os.path was incomplete back then.

I am okay with removing it but should probably be a separate PR. Unless you want to do that first and then come back to this PR? 🤷‍♀️

Unless you want to do that first and then come back to this PR? 🤷‍♀️

It makes more sense to me. Let me open a PR now.

Nevermind, I'll do it here to make sure it's done consistently (and avoid stacking PRs)

Removal of legacy things like this tend to cause havoc downstream, so it would be superb to do these removals more carefully, and at least mention them in the changelog as it would save time for those who try to investigate why packages became incompatible with the latest release (specifics are not really important, but this time you broke halotools).

There's no good reason why changes in the private API should be mentioned in the change log at all.

xref for future case studies: astropy/halotools#1107

nstarman · 2024-09-13T02:45:09Z

astropy/config/paths.py

-
-
-def get_config_dir(rootname="astropy"):
+def get_config_dir(rootname: str = "astropy") -> str:


Followup PR to return a Path object?

I depends whether this API is considered public or private: if it's private, I can do it now, and if it's public, we probably shouldn't do it at all. In doubt, I erred on the side of caution.

It looks to be public. So followup?
I think it's a very good thing to return Path objects, not string, when dealing with paths. Backwards compatibility is very easy for downstream libraries: just call str. But probably they want Paths long term :).

Dully noted. I'll circle back to this.

Let's not break downstream just because ruff is complaining here...

nstarman · 2024-09-13T02:48:04Z

astropy/config/paths.py



-def get_cache_dir(rootname="astropy"):
+def get_cache_dir(rootname: str = "astropy") -> str:


Is this function essentially the same as get_config_dir ?
Maybe a good pre or followup PR is to make a private function which these both call. Consolidate the code.

Good call. I'll make sure to do the follow up too !

for posterity, this is #17188

nstarman

LGTM. Let's get an infrastructure person to sign off on this as well.

nstarman · 2024-09-13T19:36:24Z

astropy/config/configuration.py

+                    .joinpath(pkgname)
+                    .with_suffix(".cfg")
+                )
+            cobj = configobj.ConfigObj(str(cfgfn), interpolation=False)


Also flagging for followup... maybe ConfigObj should primarily work with paths as well.

This class lives in astropy.extern so we should probably not tinker with it.

The OG project is pretty much dead. So it's not like we're going to update from extern.
We really should eventually replace the whole thing, e.g. with https://traitlets.readthedocs.io/en/stable/config.html

This seems like quite an undertaking, and I'm not sure that there would be a consensus on this as long as it's not broken. Would you like to open an issue for that ?

I think there is one...

I'll have a look in a day or two unless someone else beats me to it. I think @eteq might have done some of the original work but he is also very busy nowadays. 🤷‍♀️

pllim · 2024-09-20T18:36:39Z

astropy/config/paths.py

+        if xchpth.exists():
+            return str(xchpth.resolve())
+        else:
+            linkto = xchpth


I think in the old logic, there should also be a resolve in else?

Suggested change

linkto = xchpth

linkto = xchpth.resolve()

But looks like _find_or_create_root_dir calls resolve anyway. 🤷‍♀️

pllim · 2024-09-20T18:50:13Z

Thanks, all!

github-actions bot added testing config dev-automation labels Sep 11, 2024

neutrinoceros mentioned this pull request Sep 11, 2024

MNT: Run PTH flake test in prep for supporting pathlib #16924

Closed

neutrinoceros commented Sep 11, 2024

View reviewed changes

neutrinoceros force-pushed the config/rfc/pth_checks branch from 879a17a to 64e6c65 Compare September 11, 2024 13:12

neutrinoceros marked this pull request as ready for review September 11, 2024 13:24

neutrinoceros requested review from WilliamJamieson and nstarman as code owners September 11, 2024 13:24

eerovaher reviewed Sep 11, 2024

View reviewed changes

pllim added this to the v7.0.0 milestone Sep 11, 2024

neutrinoceros marked this pull request as draft September 12, 2024 16:45

MNT: Run PTH flake test in prep for supporting pathlib (config)

657c91a

neutrinoceros force-pushed the config/rfc/pth_checks branch from 64e6c65 to 657c91a Compare September 12, 2024 16:49

neutrinoceros marked this pull request as ready for review September 12, 2024 16:51

pllim added Build all wheels Run all the wheel builds rather than just a selection Extra CI Run cron CI as part of PR no-changelog-entry-needed labels Sep 12, 2024

nstarman reviewed Sep 13, 2024

View reviewed changes

nstarman approved these changes Sep 13, 2024

View reviewed changes

pllim reviewed Sep 20, 2024

View reviewed changes

pllim merged commit ee084c7 into astropy:main Sep 20, 2024

neutrinoceros deleted the config/rfc/pth_checks branch September 21, 2024 07:24

neutrinoceros mentioned this pull request Oct 3, 2024

ENH: expose Path-returning API in astropy.config.paths #17118

Merged

1 task

neutrinoceros mentioned this pull request Oct 16, 2024

RFC: deduplicate implementations for get_config_dir and get_cache_dir #17188

Merged

1 task

saimn mentioned this pull request Dec 6, 2024

Let user define cache location #17507

Open

pllim mentioned this pull request Dec 9, 2024

Fix configuring astropy cache location #17514

Merged

1 task



		def get_config_dir(rootname="astropy"):
		def get_config_dir(rootname: str = "astropy") -> str:



		def get_cache_dir(rootname="astropy"):
		def get_cache_dir(rootname: str = "astropy") -> str:

Uh oh!

Conversation

neutrinoceros commented Sep 11, 2024

Description

Uh oh!

github-actions bot commented Sep 11, 2024

Uh oh!

github-actions bot commented Sep 11, 2024

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!

eerovaher Sep 11, 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!

neutrinoceros Sep 13, 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!

neutrinoceros Oct 16, 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!

nstarman left a comment

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 commented Sep 20, 2024

Uh oh!

eerovaher Sep 11, 2024 •

edited

Loading

neutrinoceros Sep 13, 2024 •

edited

Loading

neutrinoceros Oct 16, 2024 •

edited

Loading