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

PXP-459 Futurize/modernize (make it python 2/3 compatible) #16

Merged
merged 27 commits into from
Oct 18, 2023

Conversation

yonran
Copy link
Contributor

@yonran yonran commented Sep 29, 2021

PXP-459

Use the futurize script and future backport library to make this library compatible with both python2 and python3. Then I manually switched from future.utils and builtins to six.moves to remove the future dependency and to avoid changing python2 behavior (urlparse).

Generally, these are the steps to using futurize:

  • Install the future~=0.18.2 library (which comes with the futurize script)
  • Run futurize --stage1 *.py remoteobjects examples tests --write --nobackups on all the files. futurize --stage1 consists of “safe” fixes: “The goal for this stage is to create most of the diff for the entire porting process, but without introducing any bugs.”, but “The output of stage 1 will probably not (yet) run on Python 3.”
  • Run futurize --stage2 --unicode-literals *.py remoteobjects examples tests --write --nobackups on all the files. futurize --stage2. Stage2 makes the code import __future__ and future backports so that hopefully it behaves the same on python2 and python3.
  • Separating text from bytes
    • Any binary packets must use binary literals b'…' and functions like bytes
    • Any python reflection must use native literals '' and functions like future.util.native_str
    • Ensure that text strings are handled consistently and optionally convert them to unicode u'…' or future’s builtins.str
  • Post-conversion: run tests and check What else you need to know for tricky cases that require manual tweaking

Here are notes specific to remoteobjects

  • I did not enable from __future__ import unicode_literals since most strings are python attributes (keys of __dict__, __get__, __setattr__, __name__, etc) which must be native_str in python2 and gives weird errors if you use unicode (e.g. error on import remoteobjects.fields: “AttributeError: 'module' object has no attribute 'fields'” because one of the functions’ __name__ was unicode instead of native_str)
  • In python2, RemoteObject Fields parsed from JSON still return str or unicode depending on what simplejson returns
  • In python3, RemoteObject Fields parsed from JSON are unicode
  • In python3, I think RemoteObject.get url and Field setter can take bytes or unicode since the underlying urllib.client.urlparse, simplejson.dumps take either bytes or unicode.
  • In python3, we UTF-8 decode before JSON loads; this replaces more invalid JSON with the replacement character than before (see new test in test_http.py)
  • I switched from future to six because six worked better in a number of ways:
    • I don’t understand the metaclasses, but future.utils.with_metaclass threw exceptions whereas six.with_metaclass worked.
    • future changes the behavior of urlparse, urljoin. In particular, it provides backports that always return unicode and then throw an exception when you mix str and unicode args (parse.py). On the other hand, six just renames the functions from python2 urllib that allow mixing unicode with str.
    • importing future.standard_library raises ImportError python 3.12, since that library is abandoned.
  • I also ran modernize to see what it would do differently. The 3 main differences were:
    • modernize added from __future__ import absolute_import to every file which is unnecessary for the most part
    • modernize omitted some list() wrappers in for x in d.keys():, which is a bug in modernize so I filed a PR Add back list() when iterating over d.items() in a for statement PyCQA/modernize#262
    • modernize used six.moves.urllib.urlparse instead of future’s backfilledurllib.urlparse. I went with modernize’s solution.

Testing

virtualenv -p python2.7 venv2
virtualenv -p python3 venv3
. venv2/bin/activate && pip install -r requirements.txt -r requirements-test.txt && python -m unittest
. venv3/bin/activate && pip install -r requirements.txt -r requirements-test.txt && python -m unittest

@yonran yonran changed the title Futurize (make it python 2/3 compatible) PXP-459 Futurize (make it python 2/3 compatible) Sep 28, 2023
@yonran yonran changed the title PXP-459 Futurize (make it python 2/3 compatible) PXP-459 Futurize/modernize (make it python 2/3 compatible) Oct 10, 2023
Copy link

@stanimoto stanimoto left a comment

Choose a reason for hiding this comment

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

Looks great!

I still don’t run doctest though since python -m doctest fails on python3 (No module named 'http.client'; 'http' is not a package
) and there are doctest errors in other files.
error: Argument 1 to "urlencode" has incompatible type "list[str]"; expected "Union[Mapping[Any, Any], Mapping[Any, Sequence[Any]], Sequence[tuple[Any, Any]], Sequence[tuple[Any, Sequence[Any]]]]"  [arg-type]
I want to call unittest.assertIsInstance
Add the test requirements to extra_require in setup.py, and use pip freeze to list the exact versions of each dependency in requirements.txt so that tests are reproducible.

Use python2 to pip freeze since the last version of pyparsing in python2 is 2.4.7 whereas it is 3.1.1 in python3
This duplicates requirements-test.txt
Add test that requirements.txt contains all the setup.py install_requires, and test that requirements.txt contains all the recursive dependencies.
“Nose has been in maintenance mode for the past several years and will likely cease without a new person/team to take over maintainership. New projects should consider using Nose2, py.test, or just plain unittest/unittest2.”
https://nose.readthedocs.io/en/latest/

We can run `python -m unittest discover` instead of nose as of python 2.7 / 3.2
https://docs.python.org/3/library/unittest.html#unittest-test-discovery

Use functools.wraps (python 2.5) instead of nose.tools.make_decorator
Fix errors given by flake8 --select=F401
Fix NameError that were caught by flake8 --select=F821, or from running mypy.
Use == and != to compare strings, not the is operator, since other implementations are not guaranteed to return True.

In python3 this fixes SyntaxWarning: "is not" with a literal. Did you mean "!="?
When overriding __eq__, call isinstance instead of type(self) == type(other), and return NotImplemented instead of returning False so that python will try the reflected comparison. This allows a different class to consider itself equal to this class.
python -m autopep8 --in-place --recursive .
E401 multiple imports on one line
W291 trailing whitespace
W391 blank line at end of file

Additionally, fix this one manually
W293 blank line contains whitespace
Use flake8 version 3.9.2 which can run from python 2.7 to 3.11 (but not python 3.12).

Use a .flake8 file that ignores pep8 whitespace issues
Mox (https://code.google.com/archive/p/pymox/wikis/MoxDocumentation.wiki) and mox3 (https://opendev.org/openstack/mox3/) are deprecated; mock (https://mock.readthedocs.io/en/latest/) is the recommended replacement. We can use mock for now and then switch to unittest.mock when we drop python<3.3 support
These old methods were considered deprecated as of python 2.7, logged DeprecationWarnings in python 3.2, and were removed in python 3.12.

assert_ -> assertTrue
failIf -> assertFalse
assertEquals -> assertEqual
self\.assertTrue\(isinstance\((.*)\)(, .*)?\) -> self.assertIsInstance($1$2)
cgi.parse_qs has been deprecated since 2.6 (2008) (https://docs.python.org/2.6/whatsnew/2.6.html, https://bugs.python.org/issue600362) and was removed in python 3.0.  2to3 and derived fixers (modernize, futurize) do not fix cgi.parse_qs; you need to move it to urlparse first.
Add support for python 3 while retaining python 2.7 compatibility. Most of the changes were automatically done by modernize and futurize. I manually undid some of the unnecessary list(d.items()) from 2to3 --fix=dict. And I had to add b'' binary literals in some places. The more complex changes to make it compatible with python 3 are in the following commits.

Add matrix to test on python 3. Add recursive dependencies to requirements.txt and requirements-test.txt.
python3 pickle checks that the __qualname__ does not contain <locals>, so we have to set a fake value in this test
AttributeError: Can't pickle local object 'TestDataObjects.set_up_pickling_class.<locals>.BasicMost'
In python2, we subclassed JSONDecoder to forgive byte sequencess that are not valid UTF-8. But in python3 we can't do that because the bytes are decoded to str prior to JSONDecoder. This change makes us decode the whole bytes ourselves before calling json.loads.
ListObject used to set 'offset' and then 'limit', but the unit test assumes that the order is limit=...&offset=.... Since dict and kwargs are guaranteed to preserve insertion order in python3.6+ (https://www.python.org/dev/peps/pep-0468/), this change makes them insert in the same order as the test expects
Fixes
AssertionError: 'http://example.com/foo?offset=0&limit=10' != 'http://example.com/foo?limit=10&offset=0'
Upgrade simplejson to 3.3.0 since prior to that, decoding json with lone surrogates would raise “JSONDecodeError: Unpaired high surrogate”
@yonran
Copy link
Contributor Author

yonran commented Oct 17, 2023

@stanimoto since your review at f7b9c1d, I added a few more minor changes to be91cde. Then I performed force pushes with equal trees from be91cde to 3116611 to 532fba7 in order to clean up history (combine the futurize steps, rearrange commits which could be applied to python2, remove references to future which is entirely replaced with six).

I don’t have permission to rebase and merge to this repo (“Only those with write access to this repository can merge pull requests.”). Would you mind doing so?

@stanimoto
Copy link

I've updated the access to this repo. Could you try it now?

@yonran yonran merged commit 48c5cda into saymedia:master Oct 18, 2023
@yonran yonran deleted the futurize branch October 18, 2023 00:07
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.

2 participants