[WIP] Python 3 support #542

pbauer · 2018-06-22T16:28:19Z

No description provided.

tisto · 2018-06-23T10:42:57Z

@pbauer wohoo! Are you sprinting remotely? Wanna do a video call connection? That worked quite well with Asko at the last sprint. :)

pbauer · 2018-06-23T11:07:52Z

Not really, I simply tried to get it running. The main issue for now is that plone.restapi tests have a hard dependency on Archetypes 😭. You should get rid of that. Then I will continue porting it to python 3.

tisto · 2018-06-23T11:12:00Z

@pbauer right. This dependency should be purely optional though. cc @lukasgraf @buchi

lukasgraf · 2018-06-23T21:19:10Z

@pbauer in what way is the AT dependency causing issues for you?
I'm asking because I was thinking that we could move it to a separate extra instead of [test], like [test_archetypes], and then adjust .travis.yml to have Travis run those separately (on Python 2.7).

But that would probably not solve your issue either I would assume. So is the only option to remove the AT dependency entirely, and have the AT parts completely untested (via CI)?

pbauer · 2018-06-24T06:48:36Z

@lukasgraf that would be good. Then we need to make all code, tests and test-layers that use archetypes optional depending on if AT can be imported or not.

lukasgraf · 2018-06-24T09:02:18Z

@pbauer I see - so it's mostly about the imports in the test code failing if AT isn't available, not so much about the actual package dependency in the [test] extra. We can make that work:

Make AT imports conditional
Skip AT tests if AT isn't available
Use ZCML conditions to only run ZCML that requires AT if it's actually present
Possibly move all the AT tests into separate Python modules

davisagli · 2018-06-24T09:04:47Z

I did a similar approach for plone.app.iterate: plone/plone.app.iterate#61

pbauer · 2018-06-28T09:03:58Z

I was a little annoyed by the merge of plone.restapi ruining the python3 build.
So I hacked at the separation from AT this morning. Instead of breakfast, so now I'm hungry 🤔

The problem was not only the tests, there also are hard imports from Products.Archetypes in restapi itself (e.g. in services/content/add.py) and several places where the code seems to bew ritten wit AT as default content in mind.

This PR is by no menas done but I have to focus on other tasks for now so please either continue working on this or start the separation and porting to python 3 fresh.

pbauer · 2018-06-28T09:05:17Z

By the way, tests of plone.restapi in py3 are currently: 863 tests, 47 failures, 135 errors, 115 skipped. Many of the failures and errors are due to the unfinished migration of plone.rest.

lukasgraf · 2018-06-28T09:06:48Z

@pbauer

hard imports from Products.Archetypes in restapi itself (e.g. in services/content/add.py) and several places where the code seems to bew ritten wit AT as default content in mind.

That is definitely unintended and we need to fix this. Thanks for your efforts!

tisto · 2018-06-28T13:23:27Z

I think we should separate the Python 3 migration from getting rid of the AT dependency. Let's do the latter first and the look into Python 3 as Philip wrote.

hvelarde · 2018-07-13T22:06:12Z

tests are failing and branch needs a rebase, but this seems to be almost ready now:

$ pylint --py3k --disable=no-absolute-import src/plone/restapi/
No config file found, using default configuration

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

hvelarde · 2018-07-13T22:14:40Z

src/plone/restapi/deserializer/atcontent.py

+    from zope.interface import Interface
+    from zope.interface import implementer
+
+    from .mixins import OrderingMixin


this should be an absolute import instead.

hvelarde · 2018-07-13T22:16:08Z

src/plone/restapi/deserializer/atcontent.py

+                if pre_errors is not None:
+                    for field_name, error_message in pre_errors.items():
+                        if field_name in errors:
+                            errors[field_name] += " %s" % error_message


shouldn't we use single quotes and .format() instead?

If we don't enforce such a style decision we shouldn't require people to stick with it IMHO.

hvelarde · 2018-07-13T22:16:49Z

src/plone/restapi/deserializer/atcontent.py

+                if post_errors is not None:
+                    for field_name, error_message in post_errors.items():
+                        if field_name in errors:
+                            errors[field_name] += " %s" % error_message


shouldn't we use single quotes and .format() instead?

hvelarde · 2018-07-13T22:24:23Z

src/plone/restapi/serializer/atcontent.py

+            if version == 'current':
+                return self.context
+            else:
+                repo_tool = getToolByName(self.context, "portal_repository")


we should use single quotes here.

hvelarde · 2018-07-13T22:25:45Z

src/plone/restapi/serializer/atfields.py

+    from zope.interface import implementer
+
+    try:
+        from Products.CMFPlone.factory import _IMREALLYPLONE5  # noqa


this # noqa needs to be more specific.

hvelarde · 2018-07-13T22:41:29Z

src/plone/restapi/tests/test_content_delete.py

@@ -79,8 +79,8 @@ def traverse(self, path='/plone', accept='application/json', method='GET'):
        request.environ['PATH_TRANSLATED'] = path
        request.environ['HTTP_ACCEPT'] = accept
        request.environ['REQUEST_METHOD'] = method
-        request._auth = 'Basic %s' % b64encode(
-            '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD))
+        auth = '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD)


shouldn't we use .format() instead?

hvelarde · 2018-07-13T22:41:41Z

src/plone/restapi/tests/test_content_delete.py

-        request._auth = 'Basic %s' % b64encode(
-            '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD))
+        auth = '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD)
+        request._auth = 'Basic %s' % b64encode(auth.encode('utf8')).decode('utf8')


shouldn't we use .format() instead?

hvelarde · 2018-07-13T22:42:08Z

src/plone/restapi/tests/test_copymove.py

@@ -34,8 +34,8 @@ def traverse(self, path='/plone', accept='application/json', method='GET'):
        request.environ['PATH_TRANSLATED'] = path
        request.environ['HTTP_ACCEPT'] = accept
        request.environ['REQUEST_METHOD'] = method
-        request._auth = 'Basic %s' % b64encode(
-            '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD))
+        auth = '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD)


shouldn't we use .format() instead?

hvelarde · 2018-07-13T22:42:17Z

src/plone/restapi/tests/test_copymove.py

-        request._auth = 'Basic %s' % b64encode(
-            '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD))
+        auth = '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD)
+        request._auth = 'Basic %s' % b64encode(auth.encode('utf8')).decode('utf8')


shouldn't we use .format() instead?

hvelarde · 2018-07-13T22:44:26Z

src/plone/restapi/tests/test_registry_serializer.py

@@ -39,7 +43,7 @@ def test_batched(self):
        obj = self.serialize(registry)
        expected = ['@id', 'items_total', 'items', 'batching']
        self.assertEqual(set(expected), set(obj.keys()))
-        self.assertEqual(obj['items_total'], len(range(1, 100)))
+        self.assertEqual(obj['items_total'], len(list(range(1, 100))))


this should be written: self.assertEqual(obj['items_total'], 99)

hvelarde · 2018-07-13T22:45:29Z

src/plone/restapi/tests/test_workflow.py

@@ -98,8 +98,8 @@ def traverse(self, path='/plone', accept='application/json',
        request.environ['PATH_TRANSLATED'] = path
        request.environ['HTTP_ACCEPT'] = accept
        request.environ['REQUEST_METHOD'] = method
-        request._auth = 'Basic %s' % b64encode(
-            '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD))
+        auth = '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD)


shouldn't we use .format() instead?

hvelarde · 2018-07-13T22:45:37Z

src/plone/restapi/tests/test_workflow.py

-        request._auth = 'Basic %s' % b64encode(
-            '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD))
+        auth = '%s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD)
+        request._auth = 'Basic %s' % b64encode(auth.encode('utf8')).decode('utf8')


shouldn't we use .format() instead?

hvelarde · 2018-07-13T22:46:16Z

src/plone/restapi/batching.py

@@ -116,7 +116,7 @@ def _url_with_params(self, params):
        # result of parse_qsl into a dict!

        # Drop params to be updated, then prepend new params in order
-        qs_params = filter(lambda x: x[0] not in params.keys(), qs_params)
+        qs_params = [x for x in qs_params if x[0] not in list(params.keys())]


this should be written: qs_params = [x for x in qs_params if x[0] not in list(params)]

davisagli · 2018-10-02T12:40:39Z

Folks, this is still a work in progress. If you want to help with that work, feel free, but otherwise please wait with the comments until it's marked ready for review.

jaroel · 2018-10-02T12:52:46Z

Ack - I'm out 👍
I've been asked to do a review and will happily do so, but I'll wait for David's signal.

davisagli · 2018-10-02T12:57:09Z

At this point the todo list before it's ready for review is:

Fix remaining test failures in Python 3 + Plone 5.2 - @davisagli
Clean up unnecessary if HAS_AT checks - @jaroel
Look into differences in docs output and figure out what to do
Check to see if any other changes got included that shouldn't be there

I put my name next to the one I'm working on; if someone else wants to work on one of the other items edit this comment and add your name to claim it.

tisto · 2018-10-02T13:26:21Z

@lukasgraf yeah. That sounds like a good idea. I still don't understand how those changes ended up in this branch in the first place. Maybe somebody accidentally committed the doc changes...

tisto · 2018-10-02T13:27:03Z

@davisagli yeah. That's correct. I will merge my branch into this one then to get it green...

tisto · 2018-10-02T13:28:32Z

Oh. Seems somebody already merged my fix. :)

tisto · 2018-10-02T17:13:33Z

I know @davisagli does not like this but I did a quick first review of this PR. Now I am not that afraid of having to do the review to be honest, which is good. Apart from the standard py3 stuff that I see I am wondering if there isn't a better way to make the AT dependency optional. So far what we do is to include AT-specific code via zcml conditionals. Since David also excluded those code fragments via if/else I suppose that is not sufficient when running things on Python 3.

@lukasgraf @buchi since you folks wrote the AT code, what is your opinion? Does the current way of excluding the AT-specific code via import + if/else makes sense to you? Would it be an alternative to factor out the AT-specific code into some kind of subpackage within plone.restapi? Or would it be an option to make a plone.restapi release without AT for Plone 5.2?

@pbauer we plan to drop AT support for Python 3, correct? Question is: is it worth the effort to support AT on Plone 5.2 / Python 2.7.

hvelarde · 2018-10-02T17:25:51Z

@tisto AT is going to be deprecated in Plone 6, not before; I see no reason to make it Python 3 compatible, but it must work on Plone 5.2 with Python 2.7.

tisto · 2018-10-02T17:50:48Z

@hvelarde we are talking about exposing AT via plone.restapi. Whatever we do with plone.restapi, it goes far beyond anything we ever promised or imagined.

jaroel · 2018-10-03T13:54:20Z

I've cleaned out as many HAS_AT checks as I could.
The ones remaining are there so I don't have to move all AT related tests to their own package - incl the layer. We'll have to remove that when AT is gone.

pbauer · 2018-10-12T15:39:34Z

AT is deprecated with Plone 5.2. It will continue to work on Python 2 with Plone 5.2 but I do not think it will still work with 6.0 unless someone else takes care of this. I would have liked to spare us the effort of supporting AT in 5.2 but then we'd have had to deprecate it in 5.1 already.

pbauer added the 22 status: in-progress label Jun 22, 2018

pbauer force-pushed the python3 branch from d773bd9 to 378ebc6 Compare June 28, 2018 04:52

tisto changed the title ~~Python 3 support~~ [WIP] Python 3 support Jun 28, 2018

sunew mentioned this pull request Jun 28, 2018

Unspecified Archetypes dependency #570

Closed

pbauer mentioned this pull request Jul 13, 2018

Port plone.rest and plone.restapi to Python 3 plone/Products.CMFPlone#2474

Closed

hvelarde reviewed Jul 13, 2018

View reviewed changes

davisagli added 3 commits October 2, 2018 09:04

Fix remaining tests

aedc7f5

Fix test: normalize difference between Plone 5.2 and previous versions

ff48344

Fix formatting

49e0137

jaroel added 6 commits October 3, 2018 09:56

gitignore mypy cache

3c36854

fix tests for local roles.

0e27792

Fix test because i'm an idiot that doesn't run them before pushing.

e3f5d65

flake8

a99b3b7

Remove HAS_AT checks as the conditional ZCML is enough

48246ac

see you later flake8r

ef79f71

jaroel and others added 5 commits October 3, 2018 15:56

flake4

5d4c94d

Skip AT tests unless we have AT

53036ea

skip tests until plone/Products.CMFPlone#2592 (comment) is resolved

ced7377

remove unused import

945cf5f

revert skip because then tne next test in the layer fails...

af02232

add crazy dummy-test to fix plone/Products.CMFPlone#2592

23fad9a

pbauer force-pushed the python3 branch from e43b46f to 23fad9a Compare October 12, 2018 16:13

add changenote and classifiers

7fe22a5

pbauer merged commit 1d4d683 into master Oct 17, 2018

pbauer deleted the python3 branch October 17, 2018 10:24

[WIP] Python 3 support #542

[WIP] Python 3 support #542

Conversation

pbauer commented Jun 22, 2018

tisto commented Jun 23, 2018

pbauer commented Jun 23, 2018

tisto commented Jun 23, 2018

lukasgraf commented Jun 23, 2018

pbauer commented Jun 24, 2018

lukasgraf commented Jun 24, 2018

davisagli commented Jun 24, 2018

pbauer commented Jun 28, 2018

pbauer commented Jun 28, 2018 • edited Loading

lukasgraf commented Jun 28, 2018

tisto commented Jun 28, 2018

hvelarde commented Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davisagli commented Oct 2, 2018

jaroel commented Oct 2, 2018

davisagli commented Oct 2, 2018 • edited by jaroel Loading

tisto commented Oct 2, 2018

tisto commented Oct 2, 2018

tisto commented Oct 2, 2018

tisto commented Oct 2, 2018

hvelarde commented Oct 2, 2018

tisto commented Oct 2, 2018

jaroel commented Oct 3, 2018

pbauer commented Oct 12, 2018

pbauer commented Jun 28, 2018 •

edited

Loading

hvelarde commented Jul 13, 2018 •

edited

Loading

davisagli commented Oct 2, 2018 •

edited by jaroel

Loading