-
Notifications
You must be signed in to change notification settings - Fork 45
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
MacOS: Implement XDG for MacOS (#4) #35
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,14 @@ | ||
/.idea/ | ||
/.venv/ | ||
/.vscode/ | ||
|
||
*.pyc | ||
*.egg-info | ||
tmp/ | ||
.coverage | ||
|
||
.tox/ | ||
build/ | ||
dist/ | ||
.tox/ | ||
tmp/ | ||
|
||
/src/platformdirs/version.py |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,20 @@ | ||
import os | ||
|
||
from .api import PlatformDirsABC | ||
from .unix import Unix | ||
|
||
XDG_SUPPORT = { | ||
"user_data_dir": "XDG_DATA_HOME", | ||
# "site_data_dir": "XDG_CONFIG_DIRS", # Not supported | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not supported? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decided against, because a quick survey of what I have installed on my Mac shows that nothing actually used the 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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). |
||
"user_config_dir": "XDG_CONFIG_HOME", | ||
# "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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because what console, please be more descriptive, but better add it right within user_log_dir not here. |
||
} | ||
|
||
class MacOS(PlatformDirsABC): | ||
|
||
class MacOS(Unix, PlatformDirsABC): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this need to extend from unix? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's from the old version, forgot to remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please fix it then 👍 |
||
""" | ||
Platform directories for the macOS operating system. Follows the guidance from `Apple documentation | ||
<https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/MacOSXDirectories/MacOSXDirectories.html>`_. | ||
|
@@ -14,7 +25,10 @@ class MacOS(PlatformDirsABC): | |
@property | ||
def user_data_dir(self) -> str: | ||
""":return: data directory tied to the user, e.g. ``~/Library/Application Support/$appname/$version``""" | ||
return self._append_app_name_and_version(os.path.expanduser("~/Library/Application Support/")) | ||
old = self._append_app_name_and_version(os.path.expanduser("~/Library/Application Support/")) | ||
if "XDG_DATA_HOME" in os.environ or not os.path.exists(old) and self.xdg_fallback: | ||
return super().user_data_dir | ||
return old | ||
|
||
@property | ||
def site_data_dir(self) -> str: | ||
|
@@ -24,7 +38,10 @@ def site_data_dir(self) -> str: | |
@property | ||
def user_config_dir(self) -> str: | ||
""":return: config directory tied to the user, e.g. ``~/Library/Preferences/$appname/$version``""" | ||
return self._append_app_name_and_version(os.path.expanduser("~/Library/Preferences/")) | ||
old = self._append_app_name_and_version(os.path.expanduser("~/Library/Preferences/")) | ||
if "XDG_CONFIG_HOME" in os.environ or not os.path.exists(old) and self.xdg_fallback: | ||
return super().user_config_dir | ||
return old | ||
|
||
@property | ||
def site_config_dir(self) -> str: | ||
|
@@ -34,12 +51,18 @@ def site_config_dir(self) -> str: | |
@property | ||
def user_cache_dir(self) -> str: | ||
""":return: cache directory tied to the user, e.g. ``~/Library/Caches/$appname/$version``""" | ||
return self._append_app_name_and_version(os.path.expanduser("~/Library/Caches")) | ||
old = self._append_app_name_and_version(os.path.expanduser("~/Library/Caches")) | ||
if "XDG_CACHE_HOME" in os.environ or not os.path.exists(old) and self.xdg_fallback: | ||
return super().user_cache_dir | ||
return old | ||
|
||
@property | ||
def user_state_dir(self) -> str: | ||
""":return: state directory tied to the user, same as `user_data_dir`""" | ||
return self.user_data_dir | ||
old = self.user_data_dir | ||
if "XDG_STATE_HOME" in os.environ or not os.path.exists(old) and self.xdg_fallback: | ||
return super().user_state_dir | ||
return old | ||
|
||
@property | ||
def user_log_dir(self) -> str: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,26 @@ | |
|
||
from .api import PlatformDirsABC | ||
|
||
# Mapping between function name and relevant XDG var | ||
XDG_SUPPORT = { | ||
"user_data_dir": "XDG_DATA_HOME", | ||
"site_data_dir": "XDG_DATA_DIRS", | ||
"user_config_dir": "XDG_CONFIG_HOME", | ||
"site_config_dir": "XDG_CONFIG_DIRS", | ||
"user_cache_dir": "XDG_CACHE_HOME", | ||
"user_state_dir": "XDG_STATE_HOME", | ||
"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 = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to document the spec compliance in a way such that it was usable in testing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
"XDG_DATA_HOME": "~/.local/share", | ||
"XDG_DATA_DIRS": "", | ||
"XDG_CONFIG_HOME": "~/.config", | ||
"XDG_CONFIG_DIRS": "", | ||
"XDG_CACHE_HOME": "~/.cache", | ||
"XDG_STATE_HOME": "~/.local/state", | ||
} | ||
|
||
|
||
class Unix(PlatformDirsABC): | ||
""" | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please no todos 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Forgot to remove. Will file another issue for it, if I remember. |
||
@property | ||
def user_log_dir(self) -> str: | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,8 +3,17 @@ | |
|
||
import appdirs | ||
import pytest | ||
from pytest_mock import MockerFixture | ||
|
||
import platformdirs | ||
from platformdirs.macos import XDG_SUPPORT as MAC_XDG_SUPPORT | ||
from platformdirs.macos import MacOS | ||
from platformdirs.unix import Unix | ||
|
||
COMPAT_PLATFORMS = { | ||
"darwin": MacOS, | ||
"unix": Unix, | ||
} | ||
|
||
|
||
def test_has_backward_compatible_class() -> None: | ||
|
@@ -28,15 +37,33 @@ def test_has_backward_compatible_class() -> None: | |
"app_name_author_version", | ||
], | ||
) | ||
def test_compatibility(params: Dict[str, Any], func: str) -> None: | ||
if sys.platform == "darwin": | ||
@pytest.mark.parametrize("system", ("darwin", "unix", "win32")) | ||
def test_compatibility( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't agree we need to touch this test in any shape and form. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the previous format the test does not validate compatibility with XDG on 🤔 |
||
mocker: MockerFixture, | ||
params: Dict[str, Any], | ||
system: str, | ||
func: str, | ||
mock_environ: Dict[str, str], | ||
) -> None: | ||
if system == "darwin": | ||
msg = { # pragma: no cover | ||
"user_log_dir": "without appname produces NoneType error", | ||
"site_config_dir": "ignores the version argument", | ||
"user_config_dir": "uses Library/Preferences instead Application Support", | ||
"user_config_dir": "uses Library/Preferences instead of Application Support", | ||
} | ||
if func in msg: # pragma: no cover | ||
pytest.skip(f"`appdirs.{func}` {msg[func]} on macOS") # pragma: no cover | ||
if MAC_XDG_SUPPORT.get(func) in mock_environ: | ||
pytest.skip(f"`appdirs.{func}` ignores the XDG specification on macOS") | ||
if system == "win32" and sys.platform != "win32": | ||
pytest.skip("Windows tests only work on win32") # pragma: no cover | ||
|
||
mocker.patch("sys.platform", system) | ||
mocker.patch("appdirs.system", system) | ||
mocker.patch("platformdirs.PlatformDirs", COMPAT_PLATFORMS[system]) | ||
# ASSUMPTION: For checking compat, we only care about the old location if | ||
# it's on disk. | ||
mocker.patch("os.path.exists", lambda _: True) | ||
|
||
new = getattr(platformdirs, func)(*params) | ||
old = getattr(appdirs, func)(*params) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
import os | ||
from pathlib import Path | ||
from textwrap import dedent | ||
from typing import Any, Dict, Tuple, Union | ||
|
||
import pytest | ||
from _pytest.fixtures import SubRequest | ||
from pytest_mock import MockerFixture | ||
|
||
from platformdirs.macos import XDG_SUPPORT, MacOS | ||
from platformdirs.unix import XDG_DEFAULTS | ||
|
||
# Mapping between function name and relevant XDG var | ||
OLD = { | ||
"user_data_dir": "~/Library/Application Support", | ||
"site_data_dir": "/Library/Application Support", | ||
"user_config_dir": "~/Library/Preferences", | ||
"site_config_dir": "/Library/Preferences", | ||
"user_cache_dir": "~/Library/Caches", | ||
"user_state_dir": "~/Library/Application Support", | ||
"user_log_dir": "~/Library/Logs", | ||
} | ||
|
||
|
||
@pytest.mark.parametrize("func", OLD.keys()) | ||
@pytest.mark.parametrize("exists", (False, True), ids=("not_exists", "exists")) | ||
@pytest.mark.parametrize( | ||
"xdg_fallback", | ||
(False, True), | ||
ids=("no_xdg_fallback", "xdg_fallback"), | ||
) | ||
def test_with_xdg_unset( | ||
mocker: MockerFixture, | ||
params: Dict[str, Any], | ||
func: str, | ||
exists: bool, | ||
xdg_fallback: bool, | ||
mock_environ: Dict[str, str], | ||
): | ||
mocker.patch("os.path.exists", lambda _: exists) | ||
obj = MacOS(**params, xdg_fallback=xdg_fallback) | ||
result = getattr(obj, func) | ||
old = os.path.expanduser(OLD[func]) | ||
|
||
if exists: | ||
prefix = old | ||
elif xdg_fallback and func in XDG_SUPPORT: | ||
prefix = os.path.expanduser(XDG_DEFAULTS[XDG_SUPPORT[func]]) | ||
else: | ||
prefix = old | ||
|
||
if not result.startswith(prefix): # pragma: no cover | ||
common = os.path.commonpath((result, prefix)) | ||
result_unique = f"${{common}}{result[len(common) :]}" | ||
prefix_unique = f"${{common}}{prefix[len(common) :]}" | ||
common_msg = f'+ where common = {common.rstrip("/")}' if common != "/" else "" | ||
msg = dedent( | ||
f"""\ | ||
Path $result does not start with $prefix | ||
+ where result = {result_unique if common_msg else result} | ||
+ where prefix = {prefix_unique if common_msg else prefix} | ||
{common_msg} | ||
""" | ||
).strip() | ||
assert False, msg |
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 do we need this map, in the unix we achieve it without doing this 🤔
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.
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.
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.
Let's remove it. Tests should not really on globals from modules, otherwise a typo here would never be caught potentially.