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

Path subclassing str #228

Merged
merged 6 commits into from
Oct 13, 2015
Merged

Path subclassing str #228

merged 6 commits into from
Oct 13, 2015

Conversation

henryiii
Copy link
Collaborator

@henryiii henryiii commented Oct 8, 2015

This patch changes path to be a subclass of str, similar to unipath. Since path was always supposed to be immutable, this was a simple change. The only api change is that cwd behaves slightly differently; due to the latest changes with the way it's now generated when you call local.cwd, this should be completely transparent. (For example, all tests pass). You now can use the as clause of a with statement to grab the new path.

Because of this change, treating the path like a string now works as expected; you can use .endswith, etc., and you can even directly open a path without converting it to a string first.

@henryiii
Copy link
Collaborator Author

henryiii commented Oct 9, 2015

By the way, to clarify my earlier statement, the only "changed" behavior is the following corner case:

mycwd = local.cwd
with mycwd('newdir'):
    print(mycwd)

Under the old system, mycwd would be the 'newdir' directory, but the new system it will still print the old dir. It does not really track the current working directory even in the old system, as any other changes to cwd through other methods will not change it. The new behavior, IMO, is actually more intuitive; and the other recommended methods work as expected:

with local.cwd('newdir'):
    print(local.cwd)

This prints the new cwd, since cwd is really a property that gets the cwd. If you want the old behavior in a more pythonic way, you can even do:

with local.cwd('newdir') as mydir:
    print(mydir)

@henryiii henryiii assigned henryiii and tomerfiliba and unassigned henryiii Oct 12, 2015
@henryiii henryiii added this to the v1.6.0 milestone Oct 12, 2015
tomerfiliba added a commit that referenced this pull request Oct 13, 2015
@tomerfiliba tomerfiliba merged commit 6fa1432 into tomerfiliba:master Oct 13, 2015
@henryiii henryiii deleted the dev branch October 13, 2015 14:18
@piotr-dobrogost
Copy link

Just for comparison quote from https://www.python.org/dev/peps/pep-0428/#no-confusion-with-builtins:

No confusion with builtins
In this proposal, the path classes do not derive from a builtin type. This contrasts with some other Path class proposals which were derived from str . They also do not pretend to implement the sequence protocol: if you want a path to act as a sequence, you have to lookup a dedicated attribute (the parts attribute).
Not behaving like one of the basic builtin types also minimizes the potential for confusion if a path is combined by accident with genuine builtin types.

@henryiii
Copy link
Collaborator Author

The downside is that then open(path) doesn't work, and that's a basic use of a path. Since historically paths are treated by strings in most languages (including the os.path functions!), having them be a string makes more sense than if they didn't have that long standing tradition of being strings.

Without str subclassing, you have to differentiate between path and str, meaning every function has to either call str() on the input, or you have to use str(path) everywhere you use a path. Certainly for a library like plumbum that focuses on clean, simple syntax, that's an added burden for new users. I do think that pathlib, being in the standard library, should add a method for being used in open() and related functions.

The downside mentioned is that something like path + 'str' will be a string, and while we can stop that if we want to, we can't stop 'str' + path. However, given my previous point, I think allowing paths to be treated as strings is okay, they just turn into strings.

Open for discussion, though, at least before 1.6.

@piotr-dobrogost
Copy link

open() and similar functions should have been able to take path from pathlib of course. I couldn't find any information on if and when it's being planned, however. You can't open(path) but you can path.open() instead :) Have you thought about making Plumbum's path api compatible with pathlib and using pathlib from stdlib on Python 3.4+?

@henryiii
Copy link
Collaborator Author

For one, that breaks duck typing. You have to try and catch then retry a different open. This is a big, known problem, but last I heard people were saying it would be 'to hard' to find and fix every open function in Python's stdlib. I haven't seen much development on pathlib since it was introduced.

Support would have to be added to more than open, too; any open function from the stdlib or 3rd party libraries like numpy and pandas would have to be patched too. A standard way to do it would make sense; the current way open works is it checks isinstance(str) and then tries a buffer interface if it doesn't work. I would rather see a special property __filename__ added to str and pathlib and any 3rd party library that wants to do paths, or have the open sequence change to try buffer first, then basically open(str(...)) if that fails. Or, maybe make an abc that all paths should register for, including pathlib...

I've been making the API's similar, but there are a few improvements that are not in pathlib. (I'm considering submitting a patch for one or two improvements to pathlib to see if there is any interest). Also, since pathlib doesn't support remotes, the two can never fully be merged. My hope is to have the api similar enough so that one can easily switch between them. Path(a_plumbum_path) definitely works, at least after the string patch (maybe before?). I haven't tried local.path(a_pathlib_path).

@henryiii
Copy link
Collaborator Author

henryiii commented Nov 3, 2015

Testing a possible fix, simply cashing the value and using that if chdir is not called. Will not allow the cd command to be run without using chdir directly, though

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

Successfully merging this pull request may close these issues.

4 participants