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

Switch from unmaintained appdirs to the drop-in replacement platformdirs #32

Merged

Conversation

CAM-Gerlach
Copy link
Contributor

@CAM-Gerlach CAM-Gerlach commented Sep 13, 2021

I couldn't open an issue or search for code use since this is marked as a fork, so I cloned the repo and went straight for a PR; hope that's okay.

In any case, for some background, I'd observed a crash in the PRAW pre-commit suite that I discovered was fixed in #30 while working on the PR we've discussed to simplify and unify the PRAW linting situation, which ironically enough apparently initially went unnoticed due to that very issue (as otherwise the CIs would have immediately flagged the issue), the linting suite on the CIs and in the pre-push script having different versions and config then the pre-commit suite and not running in isolated envs (I'll pre-commit autoupdate to pull in the fixed version).

Anyway, all that aside, given it was just added as a dep I wanted to let you know that appdirs is effectively unmaintained and platformdirs is the community's drop-in replacement.

Black, Pylint and other major projects have already switched, and given this dep was newly added, it would make sense to use the modern, maintained replacement now given its drop-in compatible and fixes numerous long-outstanding bugs with the original, instead of having to switch later when things eventually break. It also slightly simplifies the code, as platformdirs has native support for outputing pathlib.Paths.

@CAM-Gerlach CAM-Gerlach force-pushed the switch-to-platformdirs branch from 91b7766 to 84a14b7 Compare September 14, 2021 01:32
@CAM-Gerlach
Copy link
Contributor Author

CAM-Gerlach commented Sep 14, 2021

Hey @LilSpazJoekp , could you review and approve the workflow run? Thanks!

Copy link
Owner

@LilSpazJoekp LilSpazJoekp left a comment

Choose a reason for hiding this comment

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

Looks good to me! Would you like to add yourself to the AUTHORS.rst file?

@CAM-Gerlach
Copy link
Contributor Author

@LilSpazJoekp Up to you; right now it just lists the original rstformat author and the docstrfmt maintainer, so I'd presumably have to add a new section for contributors and list myself there. My typical preference is to limit the explicit listings in the manual Authors file to just authors and maintainers and include a reference to the GitHub contributors listing for the rest, which automatically tracks the rest from basic Git metadata. This minimizes manual maintenance burden and merge conflicts, avoids exposing user emails in an easier to scrape plain text file and eliminates duplication with what Git already tracks. However, I'm happy to add it if that's what you'd prefer as maintainer, just let me know.

@LilSpazJoekp
Copy link
Owner

You raise many good points. I'm good with just adding a link to the contributors on GitHub.

@LilSpazJoekp LilSpazJoekp merged commit 06115b2 into LilSpazJoekp:master Sep 15, 2021
@CAM-Gerlach
Copy link
Contributor Author

Thanks @LilSpazJoekp !

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.

2 participants