Enhance future support for Python 3 compatibility #161
Conversation
hvelarde
force-pushed
the
hvelarde-python3
branch
from
ec5a5da
to
b80aa85
Compare
| @@ -4,6 +4,7 @@ | |||
| More information: | |||
| * https://developers.facebook.com/docs/plugins | |||
| """ | |||
| from six.moves.urllib.parse import urlencode # noqa: I001 | |||
There was a problem hiding this comment.
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.
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.
| @@ -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.
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.
the fix has not being released yet: https://pypi.org/project/pylint/
| 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.
# 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
There was a problem hiding this comment.
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.
| @@ -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.
I'm not all familiar with py3k PRs. Why this is explicitly needed here?
There was a problem hiding this comment.
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).
| @@ -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.
Here, the same reasoning as sc/social/like/plugins/facebook/utils.py.
There was a problem hiding this comment.
the fix has not being released yet: https://pypi.org/project/pylint/
There was a problem hiding this comment.
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.
I'm going to ping them there.
No description provided.