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
Merged

Fix macos user_site_dir #137

merged 11 commits into from
Feb 6, 2023

Conversation

ThomasWaldmann
Copy link
Contributor

No description provided.

@ThomasWaldmann
Copy link
Contributor Author

Based on #132.

@ThomasWaldmann ThomasWaldmann changed the title Fix trailing slash (macos) Fix trailing slash (macOS) Feb 5, 2023
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@gaborbernat
Copy link
Member

- 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.
@ThomasWaldmann
Copy link
Contributor Author

ThomasWaldmann commented Feb 5, 2023

rebased #132 on fixed main, rebased this on #132.

@ThomasWaldmann ThomasWaldmann changed the title Fix trailing slash (macOS) Fix trailing slash, add tests, simplify (macOS) Feb 6, 2023
@ThomasWaldmann
Copy link
Contributor Author

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 Show resolved Hide resolved
@ThomasWaldmann ThomasWaldmann changed the title Fix trailing slash, add tests, simplify (macOS) Remove trailing slashes, add tests, simplify (macOS) Feb 6, 2023
Comment on lines 40 to 41
# 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.

Comment on lines 40 to 41
# Please note that the config dirs are NOT */Library/Preferences!
# For details see: https://github.com/platformdirs/platformdirs/issues/98
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.

CHANGES.rst Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@gaborbernat gaborbernat changed the title Remove trailing slashes, add tests, simplify (macOS) Fix macos user_site_dir Feb 6, 2023
Comment on lines 40 to 41
# Please note that the config dirs are NOT */Library/Preferences!
# For details see: https://github.com/platformdirs/platformdirs/issues/98
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.

@ThomasWaldmann
Copy link
Contributor Author

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]>
@gaborbernat gaborbernat merged commit 4fcbe17 into tox-dev:main Feb 6, 2023
Comment on lines 30 to 37
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)
Copy link
Contributor Author

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.

@ThomasWaldmann ThomasWaldmann deleted the fix-trailing-slash-macos branch February 6, 2023 21:56
@ThomasWaldmann
Copy link
Contributor Author

@gaborbernat This PR is about:

Fixing macOS *_config_dir

(see your edit of the title)

@glensc
Copy link

glensc commented Feb 7, 2023

Please fill the PR body to this PR, description, related issues, references, etc.

...Since this PR points to the BREAKING CHANGE in dependabot updates,
Well... technically Changelog doesn't point to anything, had to dig up link from Release Notes block.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants