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

Convert syntax to meet py3 standards #871

Closed
wants to merge 9 commits into from
Closed

Convert syntax to meet py3 standards #871

wants to merge 9 commits into from

Conversation

flaviut
Copy link
Contributor

@flaviut flaviut commented Sep 4, 2018

This has just a few easy-to-fix syntax error in python 3:

$ python -m compileall src build recipes/**/*.recipe manual -q
*** Error compiling 'src/calibre/db/backend.py'...
  File "src/calibre/db/backend.py", line 1628
    raise exc_info[0], exc_info[1], exc_info[2]
                     ^
SyntaxError: invalid syntax

*** Error compiling 'src/calibre/ebooks/readability/readability.py'...
  File "src/calibre/ebooks/readability/readability.py", line 159
    raise Unparseable(str(e)), None, sys.exc_info()[2]
                             ^
SyntaxError: invalid syntax

*** Error compiling 'src/calibre/gui2/icon_theme.py'...
  File "src/calibre/gui2/icon_theme.py", line 371
    raise e[0], e[1], e[2]
              ^
SyntaxError: invalid syntax

*** Error compiling 'src/calibre/gui2/linux_file_dialogs.py'...
  File "src/calibre/gui2/linux_file_dialogs.py", line 317
    raise ret[1][0], ret[1][1], ret[1][2]
                   ^
SyntaxError: invalid syntax

*** Error compiling 'src/calibre/srv/http_response.py'...
  File "src/calibre/srv/http_response.py", line 481
    raise etype, e, tb
               ^
SyntaxError: invalid syntax

*** Error compiling 'src/calibre/utils/shared_file.py'...
  File "src/calibre/utils/shared_file.py", line 122
    raise WindowsError(pywinerr.winerror, (pywinerr.funcname or '') + b': ' + (pywinerr.strerror or '')), None, sys.exc_info()[2]
                                                                                                        ^
SyntaxError: invalid syntax

*** Error compiling 'src/duktape/__init__.py'...
  File "src/duktape/__init__.py", line 160
    raise JSError(e), None, sys.exc_info()[2]
                    ^
SyntaxError: invalid syntax

I also patched python3.7/compileall.py:137 to be

        head, tail = os.path.splitext(name)
        if tail in {'.py', '.recipe'}:

in order to test the syntax of the .recipe files

This patch was tested following this process:

  • python ./setup.py bootstrap --clean-all
  • SIP_DIR=/usr/share/sip python ./setup.py bootstrap
  • python ./setup.py test
    • 1 error in test_bonjour, both before & after changes
  • python3 -m compileall src build recipes/**/*.recipe manual -q

@flaviut
Copy link
Contributor Author

flaviut commented Sep 4, 2018

Also, just a note on general principles. I am not a big fan of six. It has burned me in the past with incompatible changes and it is very heavyweight.

I haven't been using six for very long, so I haven't run into any issues. However, I did take a brief look at six.py, and I don't see anything particularly crazy.

The calibre codebase does not doa lot of fancy stuff, so what we need for compatibility is only a small subset of six.

This is definitely true.

I would prefer if we create a dedicated module for it with just what we need, much lighter weight. For an example of such a module, see python-mechanize/mechanize#9 where I wrote one for mechanize

I did take at polyglot.py, but there's a few things I like about six:

  • once py2 support is dropped, six makes it easy to just find-replace six.moves. -> <nothing>. polyglot.py imports everything into one namespace, so this is harder.
  • https://python-modernize.readthedocs.io/en/latest/ uses the six module in its automated conversions to python3, which makes the conversion much easier.

I bring this up here because six or a similar library is needed to replace raise JSError(e), None, sys.exc_info()[2]

@kovidgoyal
Copy link
Owner

six is about a thousand lines of code that takes signficant time (several milliseconds) to import. polyglot is a hundred lines of code by contrast. But, my main issue with six is that in the past it has made backwards incompatible changes that broke working code for me. For something that requires so little code to write, and that is used so widely, I prefer not adding an eternal dependency. That way we are in control of the code.

As for removing it when dropping py2 support, that is esaily done with some help from pyflakes and a little cleanup script. It also has the advantage that it is both faster (avoid extra dict lookup) and more pleasant to use (less to type).

However, I dont want you to have to do extra work for my preferences. So feel free to use six in your PRs that use the modernize library. I will take care of migrating them to polyglot.

@kovidgoyal
Copy link
Owner

Note that there is going to be a calibre release in a couple of days, I will review and merge this PR after that.

@flaviut
Copy link
Contributor Author

flaviut commented Sep 5, 2018

Sounds good to me. Ping me once you make the release, and I'll be happy to rebase onto current master.

@kovidgoyal
Copy link
Owner

The release has been made

Command used:

futurize --no-diffs -f libfuturize.fixes.fix_print_with_import -f lib2to3.fixes.fix_throw -f lib2to3.fixes.fix_numliterals -f lib2to3.fixes.fix_except -f lib2to3.fixes.fix_exec -f lib2to3.fixes.fix_raise -f lib2to3.fixes.fix_tuple_params -f lib2to3.fixes.fix_ne -j20 -w -n setup recipes src manual setup.py recipes/*.recipe

And manual adjustments of print((...)) -> print(...)
New-style rasies need to be redone in a py2-friendly way, and the
redudant imports are redudnant and can safely be removed
@kovidgoyal
Copy link
Owner

I have merged, with some minor corrections and removing the six module. Thanks.

@kovidgoyal kovidgoyal closed this Sep 10, 2018
@kovidgoyal
Copy link
Owner

As per roadmap above, I have ported the setup package to the extent that setup.py build now runs. Now it is over to you to port the C extensions. Please send a separate PR for each that I will review and merge.

@kovidgoyal
Copy link
Owner

Oh and just FYI you can build individual C extension using ./setup.py build --only

see ./setup.py build --help for details

@flaviut flaviut deleted the py3-2 branch September 10, 2018 15:52
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