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

Add pathlib support #123

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add pathlib support #123

wants to merge 2 commits into from

Conversation

Qu4tro
Copy link

@Qu4tro Qu4tro commented Mar 1, 2019

Hello there.

As you must know, Python has since 3.4 included pathlib in the standard library, as discussed in PEP 428. Long story short, it's a very nice API to the filesystem that now comes default with Python.

I understand that this software aims to have 0 dependencies and at the same time supports python >=2.7.*. This might be a bit of a blocker, but I tried to keep it as portable as possible, with minimal modifications:

  • Only AppDirs class is modified. This means, all other functions/methods preserve their types and have no relationship to pathlib.
  • AppDirs is added a new attribute use_pathlib, to determine whetever to use pathlib. It's False by default, to preserve backwards compability.
  • The property decorator is replaced by a custom decorator path_property.
  • path_property is still a property, but with an added check for the use_pathlib attribute. Only if it's true, does it import pathlib and coverts the path string to a Path object. If this branch is taken on a older Python installation, this will break, unless pathlib is explictily installed with pip.

Let me know your opinion on this.

@GhostofGoes
Copy link

Perhaps you could use a try-except import to check if pathlib is available, and instead provide an option to disable this behavior. This would make Path objects "just work" out of the box on Python 3, without affecting Python 2 in any way. The main impact would be on projects maintaining compatibility with both.

@Qu4tro
Copy link
Author

Qu4tro commented Mar 2, 2019

Sure.

I've changed the decorator to try-except the pathlib import, fallling back to the usual behavior.

One drawback of it, like you've mentioned, is that it will break for any project that doesn't have a pinned version and has pathlib available. The solution is simple though (wrapping value in str()) and maybe documenting/alerting this behavior is enough?

I like that solution, because again, like you've mentioned, we get Path objects out of the box when it is available.

@pradyunsg
Copy link

I do think the pathlib support should be an opt-in. Otherwise, this becomes a major backward incompatible change since basically everyone with Python 3 has to update all their uses and there's no way to have gradual adoption.

@McSinyx McSinyx mentioned this pull request Oct 3, 2019
@Qu4tro
Copy link
Author

Qu4tro commented Dec 16, 2019

Back to this.

@pradyunsg Would incrementing the major version be a valid compromise?

@clbarnes
Copy link

clbarnes commented Feb 19, 2020

I like @keturn 's suggestion of having a separate entry point for pathlib.Path. Changing behaviour underneath people's feet and breaking 2/3 compatibility isn't great, but it would be nice to give people access to Paths (because they're great).

As a first pass, I would subclass the AppDirs object like this in a separate module:

class AppPaths(AppDirs):
    def user_data_dir(self):
        s = super(AppPaths, self).user_data_dir()
        return Path(s)

    ...

However, to keep with the zero-dependency, vendorisable structure of this library, the whole thing would need to be done in an if block.

appdirs wouldn't benefit from Paths being great, but everyone who's making the correct decision of A) using modern python and B) using Paths, would have their choices validated and have the added convenience.

@Qu4tro
Copy link
Author

Qu4tro commented Feb 19, 2020

I like that suggestion. I don't mind implementing it, if everyone agrees.

@untzag untzag mentioned this pull request Aug 14, 2020
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.

4 participants