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.

Sign up for GitHub

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

Jump to bottom

*_config_dir -> "*/Library/Application Support" (macOS), fixes #98 #132

Closed
wants to merge 3 commits into from
Closed

*_config_dir -> "*/Library/Application Support" (macOS), fixes #98 #132

wants to merge 3 commits into from

Conversation

ThomasWaldmann
Copy link
Contributor

Also: fix a doc string.

@ThomasWaldmann
Copy link
Contributor Author

The discussion about why this should be done should be kept in the ticket, see #98.

Feedback here should be just about the code in this PR.

This was referenced Feb 5, 2023
Closed
Closed
Closed
Merged
gaborbernat
gaborbernat reviewed Feb 5, 2023
View reviewed changes
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Needs test.

@gaborbernat
Copy link
Member

We should put in a fix PR for pre commit first.

@ThomasWaldmann
Copy link
Contributor Author

ThomasWaldmann commented Feb 5, 2023 •
edited
Loading

About tests:

  • obviously there wasn't much for macOS before my PR (except the compatibility testing with appdirs, which I adjusted now)
  • the change of a string value in this PR doesn't raise test requirements above the level they were before, so i think this would be nice to have, but it is not really in-scope or rectified to demand for this PR.
  • what shall these tests actually test? _append_app_name_and_version is not macOS specific code and os.path.expanduser is stdlib code. Other than that, there is nothing in the platformdirs for macOS code.

@gaborbernat
Copy link
Member

Sorry, the requirements of contribution to this repository is not to keep the status quo. Is to leave the place in a better place than you found it 😉

@ThomasWaldmann
Copy link
Contributor Author

Well, I did. I don't feel obliged to add unrelated testing nobody cared about until now, though.

@gaborbernat
Copy link
Member

Well, I did. I don't feel obliged to add unrelated testing nobody cared about until now, though.

The lack of testing for this behavior is considered a bug, and definitely in scope for fixing #98. This is non-negotiable, sorry.

gaborbernat
gaborbernat requested changes Feb 5, 2023
View reviewed changes
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Needs changelog entry too, here https://github.com/platformdirs/platformdirs/blob/main/CHANGES.rst

@ThomasWaldmann
Copy link
Contributor Author

Under which version and release date shall the changelog entry be?

@gaborbernat
Copy link
Member

I'd recommend adding as 2.7.0 and let's say release day today.

@ThomasWaldmann
Copy link
Contributor Author

So you don't consider it breaking enough to increase major?

@gaborbernat
Copy link
Member

I guess we could make it major but is one of those that's on the edge of bug fix vs feature change. I'd be happy either way.

@ThomasWaldmann
Copy link
Contributor Author

Well, I meant it from the perspective of other projects using this code. It would mean they don't find their configs when upgrading to that version.

@gaborbernat
Copy link
Member

Sure we can make it a major too 👍

@ThomasWaldmann
*_config_dir -> "*/Library/Application Support" (macOS), fixes #98
9782dd6 
Also: fix a doc string.
@ThomasWaldmann
remove the outdated test skips for macOS
528760c 
- 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
@ThomasWaldmann
add CHANGES.rst entry
2e2a00b 
see also 2.0.0 release. :-)
@ThomasWaldmann
Copy link
Contributor Author

ThomasWaldmann commented Feb 5, 2023 •
edited
Loading

Check the 2.0.0 release changelog entry.

Rebased on fixed main.

gaborbernat
gaborbernat requested changes Feb 5, 2023
View reviewed changes
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

It still needs some tests to defend against regressions.

@ThomasWaldmann
Copy link
Contributor Author

@gaborbernat Correct tests would fail here, but I added some to #137.

@gaborbernat
Copy link
Member

@gaborbernat Correct tests would fail here, but I added some to #137.

Can you add it here so the change diff is self-container, please?

@ThomasWaldmann
Copy link
Contributor Author

@gaborbernat in case you want it all in one PR, just use #137, it includes all changesets.

@gaborbernat gaborbernat closed this Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaborbernat gaborbernat gaborbernat requested changes

@ofek ofek Awaiting requested review from ofek ofek is a code owner

@Julian Julian Awaiting requested review from Julian

@RonnyPfannschmidt RonnyPfannschmidt Awaiting requested review from RonnyPfannschmidt

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ThomasWaldmann @gaborbernat