I removed the need for `next_class` #14

Merged
merged 1 commit into from Jan 16, 2013

Projects

None yet

2 participants

@ksamuel
ksamuel commented Jan 15, 2013

It comes with the cost of a small overhead at the context manager init since we create a path object a second time but I believe it's compensated by the fact we avoid a call to self.next_class everytime.

Unit tests pass, and I added one to test how it works with the with keyword. I also added an usage example in the docstring.

@jaraco
Owner
jaraco commented on 0ab937e Jan 15, 2013

I don't think this change is acceptable. Removing the next_class property and not using self.class (as the previous implementation did) would make this change backward-incompatible. In particular, anyone who is also subclassing path wouldn't get instances of their new path class, but would only get instances of 'path'. I imagine this behavior is undesirable unless we want to remove that compatibility. I don't think we do just for tempdir. Perhaps you didn't realize the prior implementation used self.class for next_class.

Furthermore, while your implementation attempts to address next_class in the __enter__ method, it doesn't work for cases where a context manager is not used. For example:

    d = tempdir()
    (d / 'foo.txt').touch()

That will fail because (if the path class uses self.__class__ for next_class) the construction of (d / 'foo') will attempt to construct another tempdir class, which won't work.

I think we should stick with next_class.

Also, path.py still supports Python 2.3 and up - hence why I didn't include the test using a with statement. I do like the test, though, so I'd like to include it, but guard against it on Python 2.3-2.5 (we can't run it on 2.5 without the from __future__ statement, which we can't have until we require Python 2.5).

If you can re-submit your pull request to retain .next_class, but include the new test and documentation changes, I'll happily merge it in.

@jaraco
Owner
jaraco commented Jan 15, 2013

I've added two new tests in two new commits to demonstrate the desired behaviors that we should strive to retain.

@jaraco jaraco merged commit 0ab937e into jaraco:master Jan 16, 2013

1 check failed

Details default The Travis build failed
@jaraco
Owner
jaraco commented Jan 16, 2013

I apologize about the comment regarding the 'with' test. I had forgotten that the tests require Python 2.5 (even though the path.py module still supports Python 2.3). Thanks again for the contrib!

@ksamuel
ksamuel commented Jan 16, 2013

Hey, you are so quicker than I am. I didn't do anything, you did most of the work. Cool that you are so pationate about this great lib. Cheers.

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