MacOS: Implement XDG for MacOS (#4) #35

0az · 2021-08-03T05:30:25Z

This also does some minor test restructuring.

Also, while I'm on the topic of breaking changes: thoughts on making roaming, multipath, and the proposed force_xdg kwargs keyword only?

TODO: rebase onto main, and write tests for MacOS proper.

Closes #4.

gaborbernat

Also, while I'm on the topic of breaking changes: thoughts on making roaming, multipath, and the proposed force_xdg kwargs keyword only?

This will be a hard no from my side. Also no need to rework the world, your changes really should only impact the macos (and perhaps the unix - though not yet sold on that) file.

src/platformdirs/macos.py

tests/common.py

0az · 2021-08-04T22:40:47Z

Finished writing tests for coverage of the old/fallback cases for macOS. It's not 100% coverage, but it could cover the important branches pretty comprehensively.

gaborbernat

The scope of this PR is clearly well defined, and affects only macos. I'm not sure why you're touching so many unrelated files.

gaborbernat · 2021-08-04T22:42:08Z

src/platformdirs/macos.py


-class MacOS(PlatformDirsABC):
+
+class MacOS(Unix, PlatformDirsABC):


Why does this need to extend from unix?

That's from the old version, forgot to remove it.

Please fix it then 👍

gaborbernat · 2021-08-04T22:42:26Z

src/platformdirs/macos.py


+XDG_SUPPORT = {


Why do we need this map, in the unix we achieve it without doing this 🤔

It's there for the purposes of supporting testing. It's used to determine which compat tests to skip for macOS because of the introduction of XDG.

Let's remove it. Tests should not really on globals from modules, otherwise a typo here would never be caught potentially.

gaborbernat · 2021-08-04T22:42:35Z

src/platformdirs/macos.py


+XDG_SUPPORT = {
+    "user_data_dir": "XDG_DATA_HOME",
+    # "site_data_dir": "XDG_CONFIG_DIRS",  # Not supported


Why not supported?

Decided against, because a quick survey of what I have installed on my Mac shows that nothing actually used the site_*/*_DIRS variables except in vendored copies of appdirs. Actual usage of this seems to be much rarer than usage of XDG_{CONFIG,DATA,CACHE}_HOME, which I believe is the part with the most adoption in practice.

As for logging: macOS's Console.app reads from the well-known platform logging directories. As such, it's a violation of platform expectations to drop logs elsewhere.

Decided against, because a quick survey of what I have installed on my Mac shows that nothing actually used the site_*/*_DIRS variables except in vendored copies of appdirs. Actual usage of this seems to be much rarer than usage of XDG_{CONFIG,DATA,CACHE}_HOME, which I believe is the part with the most adoption in practice.

I'd rather follow the XDG spec then such local quick surveys, unless macOS clearly advises against it (as is the case of the logging).

gaborbernat · 2021-08-04T22:43:01Z

src/platformdirs/macos.py

+    # "site_config_dir": "XDG_CONFIG_DIRS",  # Not supported
+    "user_cache_dir": "XDG_CACHE_HOME",
+    "user_state_dir": "XDG_STATE_HOME",
+    # "user_log_dir": "XDG_CACHE_HOME",  # Not supported b/c Console


Because what console, please be more descriptive, but better add it right within user_log_dir not here.

gaborbernat · 2021-08-04T22:43:32Z

src/platformdirs/unix.py

@@ -93,6 +113,7 @@ def user_state_dir(self) -> str:
            path = os.path.expanduser("~/.local/state")
        return self._append_app_name_and_version(path)

+    # TODO: As per XDG spec, logs should be placed under XDG_STATE_HOME


Please no todos 👍

Forgot to remove. Will file another issue for it, if I remember.

gaborbernat · 2021-08-04T22:44:00Z

src/platformdirs/unix.py

+    "user_log_dir": "XDG_CACHE_HOME",  # Should be in XDG_STATE_HOME as per spec
+}
+# Mapping between function name and relevant XDG var
+XDG_DEFAULTS = {


You define these but doesn't seem used. The scope of this PR is to change only macos, so not sure why we need to touch even this file.

I wanted to document the spec compliance in a way such that it was usable in testing.

I'd be -1 on this. IMHO the tests should not use such globals, because effectively you're no longer testing the content of this. A typo in this silently would be ignored. Let's not do this, the test can declare it's own copy of this, so we have some sanity check in between them.

tests/test_android.py

gaborbernat · 2021-08-04T22:44:38Z

tests/test_comp_with_appdirs.py

-def test_compatibility(params: Dict[str, Any], func: str) -> None:
-    if sys.platform == "darwin":
+@pytest.mark.parametrize("system", ("darwin", "unix", "win32"))
+def test_compatibility(


I don't agree we need to touch this test in any shape and form.

I can revert that specific change and add it to a different PR. However, the test does need to be changed so that it doesn't fail on known compat breaks: i.e. when the supported subset of XDG_* is in os.environ.

In the previous format the test does not validate compatibility with XDG on 🤔

0az · 2021-08-04T23:22:10Z

Aside from the nits, what needs to be discussed is testing. I think that there's enough shared utilities to justify making a utils module under tests/, which I feel is a better structuring. At the very least, I can move the additions to .unix to it.

gaborbernat · 2021-08-04T23:24:17Z

Aside from the nits, what needs to be discussed is testing. I think that there's enough shared utilities to justify making a utils module under tests/, which I feel is a better structuring. At the very least, I can move the additions to .unix to it.

You have fixtures to share code, so you don't need utils. Also I'd prefer if this PR would not try to revamp the existing testing infrastructure, and we do that as a follow-up PR.

gaborbernat · 2021-08-27T23:05:40Z

I'll close this for now due to lack of progress, if you pick this up again I'm happy to reopen it.

0az requested review from gaborbernat, Julian, ofek and RonnyPfannschmidt as code owners August 3, 2021 05:30

0az marked this pull request as draft August 3, 2021 05:31

0az mentioned this pull request Aug 3, 2021

Allow XDG_CONFIG_HOME on macOS #4

Closed

gaborbernat requested changes Aug 3, 2021

View reviewed changes

src/platformdirs/macos.py Outdated Show resolved Hide resolved

tests/common.py Outdated Show resolved Hide resolved

0az added 5 commits August 3, 2021 14:50

Add .coverage and .venv to gitignore

e62a833

macos: Implement XDG for MacOS (platformdirs#4)

824847e

tests: Fix compat test regression

f91a841

Add suggested changes

e8ebaa2

tests: Add tests for MacOS

1531d77

0az force-pushed the mac-xdg branch from b4687ab to 1531d77 Compare August 4, 2021 22:38

0az requested a review from gaborbernat August 4, 2021 22:39

gaborbernat requested changes Aug 4, 2021

View reviewed changes

gaborbernat closed this Aug 27, 2021

percurnicus mentioned this pull request Jan 2, 2022

Allow setting custom cache directory on all platforms with environmen… psf/black#2739

Merged

3 tasks

jasongrout mentioned this pull request Sep 28, 2022

Add opt-in support for XDG_CONFIG_HOME, XDG_DATA_HOME jupyter/jupyter_core#186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MacOS: Implement XDG for MacOS (#4) #35

MacOS: Implement XDG for MacOS (#4) #35

0az commented Aug 3, 2021 •

edited

gaborbernat left a comment •

edited

0az commented Aug 4, 2021

gaborbernat left a comment

gaborbernat Aug 4, 2021

0az Aug 4, 2021

gaborbernat Aug 5, 2021

gaborbernat Aug 4, 2021

0az Aug 4, 2021

gaborbernat Aug 5, 2021

gaborbernat Aug 4, 2021

0az Aug 4, 2021

gaborbernat Aug 5, 2021

gaborbernat Aug 4, 2021

gaborbernat Aug 4, 2021

0az Aug 4, 2021

gaborbernat Aug 4, 2021

0az Aug 4, 2021

gaborbernat Aug 5, 2021

gaborbernat Aug 4, 2021

0az Aug 4, 2021

gaborbernat Aug 5, 2021

0az commented Aug 4, 2021

gaborbernat commented Aug 4, 2021 •

edited

gaborbernat commented Aug 27, 2021


		class MacOS(PlatformDirsABC):

		class MacOS(Unix, PlatformDirsABC):


		XDG_SUPPORT = {

MacOS: Implement XDG for MacOS (#4) #35

MacOS: Implement XDG for MacOS (#4) #35

Conversation

0az commented Aug 3, 2021 • edited

gaborbernat left a comment • edited

Choose a reason for hiding this comment

0az commented Aug 4, 2021

gaborbernat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0az commented Aug 4, 2021

gaborbernat commented Aug 4, 2021 • edited

gaborbernat commented Aug 27, 2021

0az commented Aug 3, 2021 •

edited

gaborbernat left a comment •

edited

gaborbernat commented Aug 4, 2021 •

edited