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

[wip] Future/pathlib - make used path type configurable #2230

Closed

Conversation

RonnyPfannschmidt
Copy link
Member

this pr is about exploring the dependency on py.path.local

the ideal case would be to completely remove the need to use py.path and instead go for pathlib/pathlib2 only

the starting point however is support, and compatibility wrappers

@RonnyPfannschmidt RonnyPfannschmidt added the type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature label Feb 3, 2017
@RonnyPfannschmidt RonnyPfannschmidt changed the base branch from master to features February 3, 2017 12:09
self._parser.addini('pathtype', 'path implementation to be used', default='pylib')

self.invocation_dir = self.make_path()
self.rootdir = self.make_path(self.rootdir)
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this would break all plugins which use this; then users are in the unfortunate situation where they can't make use of this feature because it breaks some of their plugins, and plugin authors will have to support both APIs in order to satisfy all users...

On the other hand I don't have any idea on how to solve this. 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

this is why this will be and opt-in and will support a transitional api for quite a while

however py.path has to die

the starting point in any case is compatibility wrappers and the ability to switch
only after all major plugins support the transition apis we can take a look at switching the default

this is something that will have a transition period way beyond the lifetime of python 2

Copy link
Member

Choose a reason for hiding this comment

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

this is why this will be and opt-in and will support a transitional api for quite a while
this is something that will have a transition period way beyond the lifetime of python 2

Oh OK that seems sensible, and as soon as we add an alternative the better. But what do you mean by a "transitional API" exactly? What you did here or something else entirely like a py.local subclass which supports pathlib.Path` methods?

however py.path has to die

Yeah, it had its purpose of course but with pathlib on standard library it doesn't make sense to maintain it in the long run.

Copy link
Member Author

Choose a reason for hiding this comment

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

i didnt even start with a transitional api - whats in here right now is just the opt in to change
my next steps wil lbe to experiment wit the transition

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks

Copy link
Member

Choose a reason for hiding this comment

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

Would it be simpler to add a new fixture and deprecate the tmpdir one? How many other places is py.path.local exposed?
So I assume you also looked at making py.path.local follow the __fspath__ protocol from PEP 519 and that it's not sanely doable?

Copy link
Member

Choose a reason for hiding this comment

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

How many other places is py.path.local exposed?

Unfortunately in quite a lot of places, for example the fspath attribute of test items, config.rootdir and config.inifile come to mind.

So I assume you also looked at making py.path.local follow the fspath protocol from PEP 519 and that it's not sanely doable?

That's an interesting idea, although I'm not sure if that would help us reach the ultimate goal which is to drop py.path.local altogether... at least that's what I thought the ultimate goal was.

Copy link
Member Author

Choose a reason for hiding this comment

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

fspath is already supported, but the api differs

@RonnyPfannschmidt
Copy link
Member Author

closing, i no longer believe the approach is sustainable

@RonnyPfannschmidt
Copy link
Member Author

for future reference - id like to completely switch the internals to pathlib/pathlib2

@nicoddemus
Copy link
Member

id like to completely switch the internals to pathlib/pathlib2

You and me both! For the record that was not even a possibility when py.path was created, so py.path filled a niche that there was no replacement at the time.

I would like to debate on how to replace py.path on the long term, and with the smallest disruption to users as possible.

@RonnyPfannschmidt
Copy link
Member Author

new attribute names for everything

the py.path attributes can stay for years, we can literally put the attributes into mixins for app elements and put those into a module waiting for deletion

additionally we might needthme for new objects for a while

@gaborbernat
Copy link
Contributor

As an alternative approach, what about making this part of pytest 4.0. And making it a breaking change.To be fair I would probably see this best when python 2 dies, and we can just replace it with the standard lib pathlib.

@nicoddemus
Copy link
Member

As an alternative approach, what about making this part of pytest 4.0. And making it a breaking change.

This would literally break almost every test suite that uses pytest. Imagine: any test which uses tmpdir would have to be fixed because py.path and pathlib APIs are different. And that's just one example, the py.path API is exposed publicly in a ton of places.

I imagine if we ever do that, nobody would ever update pytest in old suites, people would only use the new version with new test suites (if at all TBH).

@gaborbernat
Copy link
Contributor

True it would be a PITA to do the change; but in practice, the locations of the changes needed are fairly self-documenting and you have a good test suite pointing when all is good. So upgrading wouldn't be that much pain I believe. I'm always a bit hesitant to use the pytest paths because the API is kinda undocumented and unofficial. In many of my tests lately, I started wrapping them inside a pathlib at boundary place.

Okay so alternative approach; opt-in via a flag? Internally we can migrate all to pathlib; if the flag is set we just transform it at boundary level (e.g. return value of the fixture) to legacy py.path?

@nicoddemus
Copy link
Member

nicoddemus commented Sep 14, 2018

True it would be a PITA to do the change; but in practice, the locations of the changes needed are fairly self-documenting and you have a good test suite pointing when all is good. So upgrading wouldn't be that much pain I believe.

You are right that the breakage would be easy to spot if you have a good test suite. The problem is the sheer amount of test suites that companies have for example. At work we have easily 100+ test suites and easily 60k+ tests, it is a huge PITA to fix all this and really hard to sell that is worth the hassle.

Okay so alternative approach; opt-in via a flag?

Unfortunately plugins also use same API. If a plugin uses tmpdir with the old API, it will break if users opt-in to use the new API. 😞

@gaborbernat
Copy link
Contributor

In theory, we know all plugins (because they need to register themselves). So we could keep sending the old API to all legacy plugins, and the new one to who upgrade their plugins. It should not be hard to have opt-in flags separately for all plugins/user code; not?

@nicoddemus
Copy link
Member

nicoddemus commented Sep 14, 2018

In theory, we know all plugins (because they need to register themselves). So we could keep sending the old API to all legacy plugins, and the new one to who upgrade their plugins.

Hmmm that's a new idea, but I don't see a way to do that, for example everybody uses the same tmpdir fixture. Another example, everybody needs to access the same config object, so how can config.rootdir (today a py.path object) be a pathlib.Path to user's code and py.path to a plugin?

Besides the above, this implicit API switching going around in the background seems really hard, error prone, and make for hard to understand errors when they happen. 😬

@gaborbernat
Copy link
Contributor

They use the same tmpdir fixture, however, we inject that fixture for them into their code. Seems the appropriate place to do the transformation. I did not say it's trivial but probably the only way to do this: follow the sandwich rule -> e.g. encode/decode at system border. If the system (user-land/plugin) is compatible with ours we can just use a no-op codec.

@nicoddemus
Copy link
Member

nicoddemus commented Sep 14, 2018

I see your point, but even if we could get that to work reliably (I think it would be really hard to get it right), I believe this implicit API transformation would be just too confusing. Also because there are so many details involved, I'm sure there are corner cases where this would break down.

One example: it is common for plugins to require users to define a fixture object (say an app fixture) which the plugin expects to exist. It is certainly possible that the user might pass a Path object and the plugin expects a py.path object, and vice versa. Of course, one might argue "well but the plugin should support both!", but again this all seems error prone.

The boundary between user-code and plugins is not so clearly defined, I'm afraid.

@coveralls
Copy link

coveralls commented May 6, 2019

Coverage Status

Coverage decreased (-32.6%) to 60.134% when pulling 5289ca6 on RonnyPfannschmidt:future/pathlib into 208fae5 on pytest-dev:features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants