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

Compatibility with PythonScripts #10

Merged
merged 4 commits into from
Feb 9, 2017
Merged

Compatibility with PythonScripts #10

merged 4 commits into from
Feb 9, 2017

Conversation

thet
Copy link
Member

@thet thet commented Jan 27, 2016

Use six to access the function object and function code in zope.publisher.publisher.unwrapMethod.
This restores compatibility with Products.PythonScripts, where parameters were not extracted.

@mgedmin
Copy link
Member

mgedmin commented Jan 27, 2016

Correct me if I'm wrong -- on Python 2.6/2.7 builtin functions have both __code__ and func_code as an alias for each other, but Zope's PythonScripts still use only func_code, and this patch works because six._func_code is func_code on 2.x?

I think this corner case ought to get a unit test.

And also PythonScript should be fixed to get __code__/__func__ aliases for forwards-compatibility.

And also the Zope project as a whole needs to make a decision about continuing to support Python 3.2 now that large parts of the ecosystem no longer support it.

Do you want to raise the question of Python 3.2 support on the zope-dev@ mailing list?

@mauritsvanrees
Copy link
Member

I am working with Johannes here. The tests pass fine on Python3.2 on my laptop. I see on Travis it goes wrong because the pip version no longer supports 3.2. On a python3.2 prompt u'' gives a SyntaxError and on python3.3 this is accepted.

I will raise this on zope-dev.

@mgedmin
Copy link
Member

mgedmin commented Jan 27, 2016

In theory fixing Travis on Python 3.2 is easy: add a build step that looks like

- if [ $TOXENV = py32 ]; then pip install 'virtualenv<14' 'pip<8'; fi

(for projects that have a matrix based on Python versions instead of TOXENVs the check would be if [ $TRAVIS_PYTHON_VERSION = 3.2 ])

The question is: should we do that (maybe in a for loop for all projects), or should we just remove Python 3.2 support.

Anyway, I want to get that sorted out in master before accepting pull requests, because an unhealthy CI is a sign of an unhealthy project.

@thet
Copy link
Member Author

thet commented Mar 2, 2016

Use ``six`` to access the function object and function code in ``zope.publisher.publisher.unwrapMethod``.
This restores compatibility with Products.PythonScripts, where parameters were not extracted.
mauritsvanrees added a commit to zopefoundation/Zope that referenced this pull request Jan 31, 2017
mauritsvanrees added a commit to plone/Products.CMFFormController that referenced this pull request Jan 31, 2017
mister-roboto pushed a commit to plone/buildout.coredev that referenced this pull request Feb 4, 2017
Branch: refs/heads/master
Date: 2017-01-31T18:30:03+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/Products.CMFFormController@e6fd9b9

If func_code is None, use __code__.

See also zopefoundation/zope.publisher#10

Files changed:
M Products/CMFFormController/Script.py
Repository: Products.CMFFormController
Branch: refs/heads/master
Date: 2017-02-04T11:46:51+01:00
Author: Matthew Wilkes (MatthewWilkes) <git@matthewwilkes.name>
Commit: plone/Products.CMFFormController@2191be3

Always use __code__ and __defaults__, to match changes in CMFCore and publisher, like modern Python.

Files changed:
M CHANGES.rst
M Products/CMFFormController/ControllerPythonScript.py
M Products/CMFFormController/FSControllerPythonScript.py
M Products/CMFFormController/Script.py
M setup.py
@jensens
Copy link
Member

jensens commented Feb 6, 2017

Btw., what is missing here? I do not get it from the discussion above.

@mauritsvanrees
Copy link
Member

This is ready for merge as far as I am concerned. Could be squashed, particularly because there were two approaches here. But squashing can be done within the github UI.

Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

There are no regression tests for this change :/

Needed for current versions of Products/CMFCore/FSPythonScript.py.
@mauritsvanrees
Copy link
Member

I managed to add tests. This also showed that we needed a few more changes.

Note that we have prepared changes in CMFCore to no longer need this. But current CMFCore versions would still need it.

Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

Thank you!

@jensens jensens merged commit 0565a81 into master Feb 9, 2017
@jensens jensens deleted the thet-fix-pythonscripts branch February 9, 2017 17:59
davisagli pushed a commit to zopefoundation/Zope that referenced this pull request Feb 25, 2017
thet pushed a commit to zopefoundation/Zope that referenced this pull request Apr 3, 2017
pbauer pushed a commit to zopefoundation/Zope that referenced this pull request May 2, 2017
pbauer pushed a commit to zopefoundation/Zope that referenced this pull request May 4, 2017
jensens pushed a commit to zopefoundation/Zope that referenced this pull request May 5, 2017
MrTango pushed a commit to zopefoundation/Zope that referenced this pull request Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants