Restore compat.py in master/2.X #3443
Conversation
…it and must remain py2 compat (debugtoolbar, substanced) dont need to faff with reimplementing it
|
I did at little research before deleting it and at least the main pyramid addons I use didn't depend on it. I'm sure it broke substanced though. With respect to "yeah it was never an API" it actually was an api and removing it was part of the bw-incompat changes in 2.x. We just forgot to update the changelog (for any of the changes, even dropping py2) in #3421. I opened #3444 to remind me to do that before any release. I'm not going to put up much fight against this but I will say that I think it shouldn't be here. At the very least this module should raise a deprecation warning with an intent to remove at some point. Either that or please update Pyramid's codebase to use the functions in this module and remove the copies I added to |
|
I'm a 👎 on bringing |
Understood for personal codebases but frameworky bits like pyramid_debugtoolbar can't really just up and ditch Python 2.X unless we declare some sort of flag day. Alternately, wee could change it so it had its own compat stuff. But it's just easier for people (and us) if it doesn't break when they use it with 3.X or 2.X so we don't need to make noise Pyramid 2.X compat releases of things when it would continue to work just fine if compat was there. |
Deprecation warnings sound like a good idea to me. But as should be clear by me not just checking this in on master, I'm willing to defer too. |
Answering this part in a vacuum, unrelated to this PR: this is less of a problem than you might think thanks to the As far as this PR goes, I'm +0 merging this with deprecation warnings. |
|
It feels really weird to me to keep this around when it is no longer internally used by Pyramid, especially when we are doing a major version jump specifically because we are breaking backwards compatibility by removing Python 2 support. |
But the intent of supporting only Py 3 isn't to explicitly break Py 2 compatibility. It's not being done as a political statement, it's being done to decrease maintainer burden. If we could move to Py 3 without breaking bw compat in any way, we would. The burden of maintaining compat.py is close to zero, and if having it around keeps things working for people who want to support Py 2 and Py 3 versions of their software that depends on Pyramid, I see no practical reason to remove it. IMO, the license to break bw compat should be relatively conservative even at major version bumps, esp. when it doesn't hurt us in any way. |
|
If you want to keep it that's fine. Please either 1) add the deprecation warning and pick some version where you're cool with removing it, or 2) bring it back to the standard it was at before I removed it in #3421. It had api docs and full tests before which this PR omits. In either scenario, a decision needs to be made where the code lives. Either in |
|
I found at least that pyramid_mako is using pyramid.compat. Pylons/pyramid_mako#41 |
|
I haven't given |
|
@mcdonc what's the plan? |
|
Sorry for the late reply. What's attached now to this PR is I think the only stuff that was there before (and is required) with some updates to the docs. |
|
Awesome. We now have some duplicate methods defined in |
|
|
||
| class NewStyle(object): | ||
| def run(self): # pragma: no cover | ||
| return 'OK' |
There was a problem hiding this comment.
Python 3 only has new-style classes, some of these tests can be removed.
| ``s.decode(encoding, errors)``, otherwise return ``s``""" | ||
| if isinstance(s, binary_type): | ||
| return s.decode(encoding, errors) | ||
| return s |
There was a problem hiding this comment.
These functions are already defined in pyramid.util. Please import them from there and re-export them instead of re-defining them here.
|
reopen this if you feel like addressing comments |
Yeah it was never an API. But we used it a lot, and apps that depend on it can't immediately migrate wholesale to py3 only. It's maybe a bit fundamentalist to remove it. Maybe it should be an API.