Skip to content
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

Fix macos user_site_dir #137

Merged
merged 11 commits into from
Feb 6, 2023
4 changes: 4 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
platformdirs Changelog
======================

platformdirs 3.0.0 (2023-02-05)
ThomasWaldmann marked this conversation as resolved.
Show resolved Hide resolved
-------------------------------
- **BREAKING** Correct the config directory on OSX/macOS, again: "*/Library/Application Support" (#98)

platformdirs 2.6.2 (2022-12-28)
-------------------------------
- Fix missing ``typing-extensions`` dependency.
Expand Down
10 changes: 5 additions & 5 deletions src/platformdirs/macos.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ 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/"))
return self._append_app_name_and_version(os.path.expanduser("~/Library/Application Support"))

@property
def site_data_dir(self) -> str:
Expand All @@ -25,13 +25,13 @@ 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/"))
""":return: config directory tied to the user, same as `user_data_dir`"""
return self.user_data_dir

@property
def site_config_dir(self) -> str:
""":return: config directory shared by the users, e.g. ``/Library/Preferences/$appname``"""
return self._append_app_name_and_version("/Library/Preferences")
""":return: config directory shared by the users, same as `site_data_dir`"""
return self.site_data_dir

@property
def user_cache_dir(self) -> str:
Expand Down
4 changes: 1 addition & 3 deletions tests/test_comp_with_appdirs.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ def test_compatibility(params: dict[str, Any], func: str) -> None:
if sys.platform == "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",
}
if func in msg: # pragma: no cover
pytest.skip(f"`appdirs.{func}` {msg[func]} on macOS") # pragma: no cover
Expand All @@ -72,4 +70,4 @@ def test_compatibility(params: dict[str, Any], func: str) -> None:
new = getattr(platformdirs, func)(*params)
old = getattr(appdirs, func)(*params)

assert new == old
assert new == old.rstrip("/")
52 changes: 52 additions & 0 deletions tests/test_macos.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
from __future__ import annotations

import os
from typing import Any

import pytest

from platformdirs.macos import MacOS


@pytest.mark.parametrize(
"params",
[
{},
{"appname": "foo"},
{"appname": "foo", "version": "v1.0"},
],
ids=[
"no_args",
"app_name",
"app_name_version",
],
)
def test_macos(params: dict[str, Any], func: str) -> None:
result = getattr(MacOS(**params), func)

home = os.path.expanduser("~")
suffix_elements = []
if "appname" in params:
suffix_elements.append(params["appname"])
if "version" in params:
suffix_elements.append(params["version"])
if suffix_elements:
suffix_elements.insert(0, "")
suffix = "/".join(suffix_elements)

expected_map = {
"user_data_dir": f"{home}/Library/Application Support{suffix}",
"site_data_dir": f"/Library/Application Support{suffix}",
# Please note that the config dirs are NOT */Library/Preferences!
# For details see: https://github.com/platformdirs/platformdirs/issues/98
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess this should avoid somebody changing this back to the wrong dirs again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this comment? The test itself is enough with the git blame.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering this was initially correct, then broken for platformdirs 2.0.0 and now back in the initial correct state, I would strongly suggest we keep that comment, so that this never happens again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove.

Copy link
Contributor Author

@ThomasWaldmann ThomasWaldmann Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree and I'ld appreciate another persons opinion here.

You can't just assume that the next guy who breaks this will do git blame or a lot of other research first. Otherwise the breakage at 2.0.0 would also not have happened.

And if such a comment would have been there before the 2.0.0 breakage, we would not have had all 2.x.x releases wrong on macOS and also would not have had 2 incompatible/breaking releases in total for macOS users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look, the maintainer's opinion trumps your opinion 😊 at the end of the day.

"user_config_dir": f"{home}/Library/Application Support{suffix}",
"site_config_dir": f"/Library/Application Support{suffix}",
"user_cache_dir": f"{home}/Library/Caches{suffix}",
"user_state_dir": f"{home}/Library/Application Support{suffix}",
"user_log_dir": f"{home}/Library/Logs{suffix}",
"user_documents_dir": f"{home}/Documents",
"user_runtime_dir": f"{home}/Library/Caches/TemporaryItems{suffix}",
}
expected = expected_map[func]

assert result == expected