-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix macos user_site_dir #137
Conversation
Based on #132. |
Also: fix a doc string.
- that it doesn't include the version number was not the case (this is unrelated to my change) - it is back to Application Support, so that does not apply any more due to my change
see also 2.0.0 release. :-)
needed to adjust the compatibility-with-appdirs test, because appdirs has the same issue.
Considering I actually changed something here in the way the paths are made up (removed the trailing slash), I decided to add macOS tests here. The (IMHO) correct tests added here would have failed in #132 due to the trailing slashes. |
tests/test_macos.py
Outdated
# Please note that the config dirs are NOT */Library/Preferences! | ||
# For details see: https://github.com/platformdirs/platformdirs/issues/98 |
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.
guess this should avoid somebody changing this back to the wrong dirs again.
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.
Can you remove this comment? The test itself is enough with the git blame.
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.
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.
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.
Please remove.
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.
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.
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.
Look, the maintainer's opinion trumps your opinion 😊 at the end of the day.
tests/test_macos.py
Outdated
# Please note that the config dirs are NOT */Library/Preferences! | ||
# For details see: https://github.com/platformdirs/platformdirs/issues/98 |
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.
Can you remove this comment? The test itself is enough with the git blame.
tests/test_macos.py
Outdated
# Please note that the config dirs are NOT */Library/Preferences! | ||
# For details see: https://github.com/platformdirs/platformdirs/issues/98 |
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.
Please remove.
Oh, it runs the macOS tests also on windows (and likely also on every other OS). I'll fix that. |
Signed-off-by: Bernát Gábor <[email protected]>
Signed-off-by: Bernát Gábor <[email protected]>
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) |
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.
BTW, that was c&p from another test module. So guess you want to "fix" it there, too.
@gaborbernat This PR is about:
(see your edit of the title) |
Please fill the PR body to this PR, description, related issues, references, etc. ...Since this PR points to the BREAKING CHANGE in dependabot updates, It's unclear to me what was the BEFORE value? change is just removing of trailing slash? if so, why is trailing slash removal a breaking change? |
No description provided.