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

Try and fake pathlib2 before pathlib #462

Closed
peteboothroyd opened this issue Dec 21, 2018 · 11 comments
Closed

Try and fake pathlib2 before pathlib #462

peteboothroyd opened this issue Dec 21, 2018 · 11 comments

Comments

@peteboothroyd
Copy link

Is your feature request related to a problem? Please describe.
pathlib2 is only used if pathlib is not found. As pathlib is now included for python>=3.4 pathlib2 will not be faked for these newer versions of python even if it is present in a project. As I understand it the 'correct' usage of pathlib2 is not to use conditional imports such as in the extra_packages.py as highlighted in this issue on the pathlib2 repo, but rather to use pathlib2 directly in place of pathlib where compatibility between different python versions is required.

This means (AFAIK) that if I want to use pathlib2 in my project and have it faked I have to use the Patcher and modules_to_patch option - and then have to reload certain modules as a result.

Describe the solution you'd like
I think it would be better to flip the logic in extra_packages.py to first try and load pathlib2 and then only if this is not present to load pathlib. pathlib2 has to be explicitly downloaded whereas pathlib is included in the standard library so there should be fewer instances where the wrong module is patched. Alternatively I don't know if it would be possible/advisable to patch both modules with the faked module?

@mrbean-bremen
Copy link
Member

I think it would be better to flip the logic in extra_packages.py to first try and load pathlib2 and then only if this is not present to load pathlib.

This sounds reasonable, I have to think a bit about this.

Alternatively I don't know if it would be possible/advisable to patch both modules with the faked module?

I don't think this would be good idea - you don't want to mix both libraries in the same project, so that should not be needed.

I will look at this some time later...

@mrbean-bremen
Copy link
Member

Ok, I just checked the pathlib2 issue you mentioned, and agree that the change certainly would make sense.
I may have a look somewhere in the next week (or in the next year...)

@mrbean-bremen
Copy link
Member

@peteboothroyd - I changed the order of import as proposed - please check if this is what you need.

@peteboothroyd
Copy link
Author

I think there are some errors in fake_pathlib.py as some of the classes inherit from pathlib.Path, however if pathlib2 is imported successfully then pathlib is set to None.

@mrbean-bremen
Copy link
Member

Ah, ok - I don't understand why this did not come up in the tests. Hm, maybe it will happen only for Python>3.4, where pathlib2 is not tested - I will check this.

mrbean-bremen added a commit that referenced this issue Jan 2, 2019
- adapted tests to test pathlib2 and scandir package in all versions
- see #462
@mrbean-bremen
Copy link
Member

Ok, I changed the code accordingly - hopefully correctly this time. Please check again.

@peteboothroyd
Copy link
Author

Yes that seems to be working perfectly for me! Appreciate the help :)

@peteboothroyd
Copy link
Author

Would it be possible to get a new PyPi release?

@mrbean-bremen
Copy link
Member

mrbean-bremen commented Jan 3, 2019

Hm, maybe I will wait a bit to check if anything else comes up...

@mrbean-bremen
Copy link
Member

Ok, nothing came up, so I made a new PyPi release (something will come up tomorrow for sure...)
Closing.

@mrbean-bremen
Copy link
Member

@peteboothroyd - I made some changes to the pathlib handling in master due to a regression I introduced with the changes for this issue. It would be nice if you could check if the current master still works for you (regarding pathlib/pathlib3), before I make another release with the change.

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

No branches or pull requests

2 participants