-
Notifications
You must be signed in to change notification settings - Fork 23
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
Changes from all commits
cea632c
a8e8d2d
bb3cb8c
237e72c
2f2272f
60c1af9
b80aa85
3d9fd46
f3f0e54
bd3807c
262d271
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because exceptions in Python 3 have no
|
||
|
||
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): | ||
|
@@ -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': | ||
|
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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, the same reasoning as sc/social/like/plugins/facebook/utils.py. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the fix has not being released yet: https://pypi.org/project/pylint/ There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm going to ping them there. |
||
|
||
|
||
class To3042TestCase(UpgradeTestCaseBase): | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.