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

Enhance future support for Python 3 compatibility #161

Merged
merged 11 commits into from
Jun 6, 2018
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ before_script:
script:
- bin/code-analysis
- bin/test
- bin/pylint --py3k --disable=no-absolute-import sc/social/like || true
- bin/pylint --py3k --disable=no-absolute-import sc/social/like
after_success:
- bin/zopepy setup.py check -mrs
- pip install coverage
Expand Down
4 changes: 4 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ There's a frood who really knows where his towel is.
2.13b4 (unreleased)
^^^^^^^^^^^^^^^^^^^

- Code refactor to increase future Python 3 compatibility;
add dependency on `six <https://pypi.python.org/pypi/six>`_.
[hvelarde]

- Fix upgrade step when there are no plone.app.tiles recrods.
[erral]

Expand Down
2 changes: 1 addition & 1 deletion sc/social/like/plugins/facebook/browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
More information:
* https://developers.facebook.com/docs/plugins
"""
from six.moves.urllib.parse import urlencode # noqa: I001
Copy link
Member

Choose a reason for hiding this comment

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

Does six recommend it's imports to be on the top of the file? (A lot of files in this PR have this same pattern but I'm not going to comment in all of them)

Copy link
Member Author

Choose a reason for hiding this comment

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

no, but I'm putting it above just to make it easier to remove it in the future when we drop support for Python 2.

from Acquisition import aq_inner
from plone import api
from Products.Five import BrowserView
Expand All @@ -13,7 +14,6 @@
from sc.social.like.interfaces import ISocialLikeSettings
from sc.social.like.plugins.facebook.utils import facebook_language
from sc.social.like.utils import get_language
from urllib import urlencode


class PluginView(BrowserView):
Expand Down
3 changes: 2 additions & 1 deletion sc/social/like/plugins/facebook/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,6 @@ def facebook_language(languages, default):
return default
languages = [languages] if not isinstance(languages, list) else languages
languages = [fix_iso(l) for l in languages]
prefered = [l for l in languages if l in FB_LOCALES]
# XXX: https://github.com/PyCQA/pylint/issues/2106
prefered = [l for l in languages if l in FB_LOCALES] # noqa: E501; pylint: disable=comprehension-escape
Copy link
Member

Choose a reason for hiding this comment

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

Do you use E501 because of the pylint: disable=comprehension-escape? Didn't they create a new release of pylint that fixes this problem in pylint-dev/pylint#2106?

Copy link
Member Author

Choose a reason for hiding this comment

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

the fix has not being released yet: https://pypi.org/project/pylint/

return prefered and prefered[0] or default
2 changes: 1 addition & 1 deletion sc/social/like/plugins/gplus/browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@
More information:
* https://developers.google.com/+/web/share/
"""
from six.moves.urllib.parse import urlencode # noqa: I001
from Acquisition import aq_inner
from plone import api
from Products.Five import BrowserView
from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile
from sc.social.like.interfaces import ISocialLikeSettings
from sc.social.like.utils import get_language
from urllib import urlencode


class PluginView(BrowserView):
Expand Down
2 changes: 1 addition & 1 deletion sc/social/like/plugins/linkedin/browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
* https://developer.linkedin.com/plugins/share
* https://developer.linkedin.com/docs/share-on-linkedin
"""
from six.moves.urllib.parse import urlencode # noqa: I001
from Acquisition import aq_inner
from plone import api
from Products.Five import BrowserView
from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile
from sc.social.like.interfaces import ISocialLikeSettings
from sc.social.like.utils import get_language
from urllib import urlencode


class PluginView(BrowserView):
Expand Down
2 changes: 1 addition & 1 deletion sc/social/like/plugins/pinterest/browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
* https://developers.pinterest.com/docs/widgets/save/
* https://developers.pinterest.com/docs/rich-pins/reference/
"""
from six.moves.urllib.parse import urlencode # noqa: I001
from Acquisition import aq_inner
from plone import api
from Products.Five import BrowserView
from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile
from sc.social.like.interfaces import ISocialLikeSettings
from sc.social.like.utils import get_content_image
from urllib import urlencode


class PluginView(BrowserView):
Expand Down
2 changes: 1 addition & 1 deletion sc/social/like/plugins/twitter/browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
* https://dev.twitter.com/web/tweet-button
* https://dev.twitter.com/web/overview/privacy
"""
from six.moves.urllib.parse import urlencode # noqa: I001
from Acquisition import aq_inner
from plone import api
from Products.Five import BrowserView
from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile
from sc.social.like.behaviors import ISocialMedia
from sc.social.like.config import IS_PLONE_5
from sc.social.like.interfaces import ISocialLikeSettings
from urllib import urlencode


class PluginView(BrowserView):
Expand Down
21 changes: 8 additions & 13 deletions sc/social/like/subscribers.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
from sc.social.like.utils import validate_og_lead_image
from sc.social.like.utils import validate_og_title
from zope.component import getUtility
from zope.schema.interfaces import WrongType

import requests
import traceback
Expand Down Expand Up @@ -57,10 +56,10 @@ def social_media_record_synchronizer(event):
if not IS_PLONE_5:
return

logger.debug(u'Processing: ' + repr(event.record))
logger.debug('Processing: ' + repr(event.record))
field = event.record.fieldName
if field not in FIELDS:
logger.debug(u'Field name not being tracked')
logger.debug('Field name not being tracked')
return

# find out which record we need to synchronize
Expand All @@ -73,16 +72,12 @@ def social_media_record_synchronizer(event):
# Plone record modified; synchronize sc.social.like record
record = ISocialLikeSettings.__identifier__ + '.' + field
else:
logger.debug(u'Schema not being tracked')
logger.debug('Schema not being tracked')
return

registry = getUtility(IRegistry)
# this will fire the aditional IRecordModifiedEvent
try:
registry[record] = str(event.record.value)
except WrongType:
# Plone 5 declares records as TextLine
registry[record] = unicode(event.record.value)
registry[record] = str(event.record.value)
Copy link
Member

Choose a reason for hiding this comment

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

# Plone 5 declares records as TextLine		
registry[record] = unicode(event.record.value)

Funny, this was supposed to make it ok in Plone 5 but removing it the test is still ok https://travis-ci.org/collective/sc.social.like/jobs/385858397#L1725

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 fixed that for Plone 5.1 and I'm dropping support for Plone 5.0; in fact the package is still not compatible with Plone 5 at all, but I want to simplify the code.


logger.debug('{0} was synchronized; new value is "{1}"'.format(
repr(registry.records[record]), event.record.value))
Expand Down Expand Up @@ -138,19 +133,19 @@ def check_sharing_best_practices(obj, event):
try:
validate_og_title(title)
except ValueError as e:
api.portal.show_message(message=e.message, request=request, type='warning')
api.portal.show_message(message=str(e), request=request, type='warning')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not all familiar with py3k PRs. Why this is explicitly needed here?

Copy link
Member Author

Choose a reason for hiding this comment

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

because exceptions in Python 3 have no message attribute; there's a convention to use the __str__ method to print that information since Python 2:

The string printed as the exception type is the name of the built-in exception that occurred. This is true for all built-in exceptions, but need not be true for user-defined exceptions (although it is a useful convention).

https://docs.python.org/2/tutorial/errors.html#exceptions


description = getattr(obj, 'description', '')
try:
validate_og_description(description)
except ValueError as e:
api.portal.show_message(message=e.message, request=request, type='warning')
api.portal.show_message(message=str(e), request=request, type='warning')

image = get_content_image(obj)
try:
validate_og_lead_image(image)
except ValueError as e:
api.portal.show_message(message=e.message, request=request, type='warning')
api.portal.show_message(message=str(e), request=request, type='warning')


def facebook_prefetching(obj, event):
Expand All @@ -176,7 +171,7 @@ def facebook_prefetching(obj, event):
try:
r = requests.post(endpoint, timeout=5)
except requests.exceptions.RequestException as e:
logger.warn('Prefetch failure: ' + str(e.message))
logger.warn('Prefetch failure: ' + str(e))
return

if r.status_code == '200':
Expand Down
7 changes: 1 addition & 6 deletions sc/social/like/tests/test_subscribers.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,7 @@ def test_modify_social_like_settings(self):

@unittest.skipIf(not IS_PLONE_5, 'This test is for Plone 5 only')
def test_modify_plone_settings(self):
from zope.schema.interfaces import WrongType
try:
self.registry['plone.twitter_username'] = 'hvelarde'
except WrongType:
# in Plone 5.0 this field type was TextLine
self.registry['plone.twitter_username'] = u'hvelarde'
self.registry['plone.twitter_username'] = 'hvelarde'
self.assertEqual(self.settings.twitter_username, 'hvelarde')
# changing other fields don't break anything
self.registry['plone.share_social_data'] = False
Expand Down
5 changes: 3 additions & 2 deletions sc/social/like/tests/test_upgrades.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# -*- coding: utf-8 -*-
from six.moves import range # noqa: I001
from plone import api
from plone.registry.interfaces import IRegistry
from sc.social.like.config import IS_PLONE_5
Expand Down Expand Up @@ -189,7 +190,7 @@ def test_register_cover_tiles(self):
self.execute_upgrade_step(step)

registered = api.portal.get_registry_record('plone.app.tiles')
[self.assertIn(t, registered) for t in TILES]
[self.assertIn(t, registered) for t in TILES] # noqa: E501; pylint: disable=W1662
Copy link
Member

Choose a reason for hiding this comment

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

Here, the same reasoning as sc/social/like/plugins/facebook/utils.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

the fix has not being released yet: https://pypi.org/project/pylint/

Copy link
Member

Choose a reason for hiding this comment

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

You're right. Although unrelated to this PR, that's why we still have these comments in the IDG project as well. I thought they created a release since you said in a comment it's working in the related issue.

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'm going to ping them there.



class To3042TestCase(UpgradeTestCaseBase):
Expand Down Expand Up @@ -308,7 +309,7 @@ def test_reindex_catalog(self):
from sc.social.like.behaviors import ISocialMedia
from sc.social.like.tests.utils import enable_social_media_behavior
with api.env.adopt_roles(['Manager']):
for i in xrange(0, 10):
for i in range(0, 10):
api.content.create(self.portal, 'News Item', str(i))

# break the catalog by deleting an object without notifying
Expand Down
12 changes: 6 additions & 6 deletions sc/social/like/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def test_validate_og_title_invalid(self):
try:
validate_og_description(title)
except ValueError as e:
self.assertEqual(e.message, MSG_INVALID_OG_TITLE)
self.assertEqual(str(e), MSG_INVALID_OG_TITLE)

def test_validate_og_description_valid(self):
self.assertTrue(validate_og_description(None))
Expand All @@ -83,7 +83,7 @@ def test_validate_og_description_invalid(self):
try:
validate_og_description(description)
except ValueError as e:
self.assertEqual(e.message, MSG_INVALID_OG_DESCRIPTION)
self.assertEqual(str(e), MSG_INVALID_OG_DESCRIPTION)

def test_validate_og_lead_image_no_image(self):
self.assertTrue(validate_og_lead_image(None))
Expand All @@ -105,7 +105,7 @@ def test_validate_og_lead_image_invalid_mime_type(self):
try:
validate_og_lead_image(image)
except ValueError as e:
self.assertEqual(e.message, MSG_INVALID_OG_LEAD_IMAGE_MIME_TYPE)
self.assertEqual(str(e), MSG_INVALID_OG_LEAD_IMAGE_MIME_TYPE)

def test_validate_og_lead_image_invalid_size(self):
from sc.social.like.config import OG_LEAD_IMAGE_MAX_SIZE
Expand All @@ -121,7 +121,7 @@ def test_validate_og_lead_image_invalid_size(self):
try:
validate_og_lead_image(image)
except ValueError as e:
self.assertEqual(e.message, MSG_INVALID_OG_LEAD_IMAGE_SIZE)
self.assertEqual(str(e), MSG_INVALID_OG_LEAD_IMAGE_SIZE)

def test_validate_og_lead_image_invalid_dimensions(self):
from sc.social.like.utils import MSG_INVALID_OG_LEAD_IMAGE_DIMENSIONS
Expand All @@ -135,7 +135,7 @@ def test_validate_og_lead_image_invalid_dimensions(self):
try:
validate_og_lead_image(image)
except ValueError as e:
self.assertEqual(e.message, MSG_INVALID_OG_LEAD_IMAGE_DIMENSIONS)
self.assertEqual(str(e), MSG_INVALID_OG_LEAD_IMAGE_DIMENSIONS)

def test_validate_og_lead_image_invalid_aspect_ratio(self):
from sc.social.like.utils import MSG_INVALID_OG_LEAD_IMAGE_ASPECT_RATIO
Expand All @@ -149,7 +149,7 @@ def test_validate_og_lead_image_invalid_aspect_ratio(self):
try:
validate_og_lead_image(image)
except ValueError as e:
self.assertEqual(e.message, MSG_INVALID_OG_LEAD_IMAGE_ASPECT_RATIO)
self.assertEqual(str(e), MSG_INVALID_OG_LEAD_IMAGE_ASPECT_RATIO)


def load_tests(loader, tests, pattern):
Expand Down
13 changes: 7 additions & 6 deletions sc/social/like/tests/test_viewlets.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# -*- coding: utf-8 -*-
from six.moves import range # noqa: I001
from plone import api
from profilehooks import profile
from profilehooks import timecall
Expand Down Expand Up @@ -27,10 +28,10 @@ def capture():
http://stackoverflow.com/a/10743550/644075
"""
import sys
from cStringIO import StringIO
from io import BytesIO
oldout, olderr = sys.stdout, sys.stderr
try:
out = [StringIO(), StringIO()]
out = [BytesIO(), BytesIO()]
sys.stdout, sys.stderr = out
yield out
finally:
Expand Down Expand Up @@ -131,7 +132,7 @@ def test_metadata_viewlet_rendering_performance(self):

@timecall(immediate=True)
def render(times):
for i in xrange(0, times):
for i in range(0, times):
viewlet.render()

with capture() as out:
Expand All @@ -143,7 +144,7 @@ def render(times):
# show rendering profile
@profile
def render(times):
for i in xrange(0, times):
for i in range(0, times):
viewlet.render()

render(times)
Expand Down Expand Up @@ -211,7 +212,7 @@ def test_social_viewlet_rendering_performance(self):

@timecall(immediate=True)
def render(times):
for i in xrange(0, times):
for i in range(0, times):
viewlet.render()

with capture() as out:
Expand All @@ -223,7 +224,7 @@ def render(times):
# show rendering profile
@profile
def render(times):
for i in xrange(0, times):
for i in range(0, times):
viewlet.render()

render(times)
3 changes: 2 additions & 1 deletion sc/social/like/tests/utils.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# -*- coding: utf-8 -*-
from six.moves import range # noqa: I001
from plone.dexterity.interfaces import IDexterityFTI
from plone.dexterity.schema import SchemaInvalidatedEvent
from sc.social.like.behaviors import ISocialMedia
Expand All @@ -18,7 +19,7 @@ def enable_social_media_behavior():
def get_random_string(length):
from random import choice
from string import printable
return ''.join(choice(printable) for i in xrange(0, length))
return ''.join(choice(printable) for i in range(0, length))


def get_file(filename):
Expand Down
2 changes: 1 addition & 1 deletion sc/social/like/tiles/facebook.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
For additional implementation detail see:
https://developers.facebook.com/docs/plugins/page-plugin
"""
from six.moves.urllib.parse import urlencode # noqa: I001
from collective.cover.tiles.base import IPersistentCoverTile
from collective.cover.tiles.base import PersistentCoverTile
from plone import api
Expand All @@ -12,7 +13,6 @@
from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile
from sc.social.like import LikeMessageFactory as _
from sc.social.like.interfaces import ISocialLikeSettings
from urllib import urlencode
# from z3c.form.browser.checkbox import CheckBoxFieldWidget
from zope import schema
from zope.interface import implementer
Expand Down