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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Python 2 support #2649

Merged
merged 5 commits into from Nov 26, 2019

Conversation

@iKevinY
Copy link
Member

iKevinY commented Nov 6, 2019

This PR is a first-pass at removing Python 2 support for Pelican, as described in the #2645 metabug. Specifically, I went through and removed all instances of six, as well as utils.python_2_unicode_compatible. While I did try to remove Python 2-specific logic whenever I saw it, I wasn't searching carefully for it (this will likely require a finer comb through the codebase), nor has documentation been updated either.

(I pulled in #2647 so that Travis wouldn't complain about failing Python 2 builds. 馃槄)

@iKevinY iKevinY mentioned this pull request Nov 6, 2019
7 of 7 tasks complete
@iKevinY iKevinY marked this pull request as ready for review Nov 6, 2019
Copy link
Member

avaris left a comment

This is from just looking at the PR changes list. Probably more will come when I can go through the whole code in detail :).

pelican/__init__.py Outdated Show resolved Hide resolved
pelican/log.py Show resolved Hide resolved
pelican/log.py Outdated Show resolved Hide resolved
pelican/utils.py Show resolved Hide resolved
poetry.lock Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@iKevinY

This comment has been minimized.

Copy link
Member Author

iKevinY commented Nov 7, 2019

Thanks for the initial review @avaris! I'll take a closer pass through the codebase again tonight to remove some more of the subtle Python 2 stuff.

@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Nov 16, 2019

@avaris: Any chance you might be able to go through the code in detail this weekend? It'd be great to get this PR, and your namespace plugin PR, merged soon. 馃槄

@avaris

This comment has been minimized.

Copy link
Member

avaris commented Nov 16, 2019

@justinmayer that's the plan :).

@avaris

This comment has been minimized.

Copy link
Member

avaris commented Nov 16, 2019

Below are Python2 related code. These might include parts touched by this PR but not completely cleaned. I'm putting them all here for completeness sake, instead of adding comments to lines.

  • try:
    # spec_from_file_location is the recommended way in Python 3.5+
    import importlib.util
    def load_source(name, path):
    spec = importlib.util.spec_from_file_location(name, path)
    mod = importlib.util.module_from_spec(spec)
    spec.loader.exec_module(mod)
    return mod
    except ImportError:
    # but it does not exist in Python 2.7, so fall back to imp
    import imp
    load_source = imp.load_source
  • Unnecessary:
    from codecs import open
  • Other codecs imports:
    • tests/test_importer.py
    • tests/test_generators.py
    • tools/pelican_import.py
    • tools/pelican_quickstart.py
  • This is kind of the opposite (py3 only code :) ) but it is fixed in docutils 0.15, should be fine to remove if we fix dependecy:

    pelican/pelican/readers.py

    Lines 206 to 217 in bae6de5

    class FileInput(docutils.io.FileInput):
    """Patch docutils.io.FileInput to remove "U" mode in py3.
    Universal newlines is enabled by default and "U" mode is deprecated
    in py3.
    """
    def __init__(self, *args, **kwargs):
    if six.PY3:
    kwargs['mode'] = kwargs.get('mode', 'r').replace('U', '')
    docutils.io.FileInput.__init__(self, *args, **kwargs)
  • Currently, this is changed to str(...) but strifying is not needed at all:
    return six.text_type(slugify(key, regex_subs=subs))
  • from cgi ... is Python2-only:

    pelican/pelican/utils.py

    Lines 34 to 37 in bae6de5

    try:
    from html import escape
    except ImportError:
    from cgi import escape
  • super.__init__(convert_charrefs=False) should be fine for all this:

    pelican/pelican/utils.py

    Lines 461 to 467 in bae6de5

    # In Python 2, HTMLParser is not a new-style class,
    # hence super() cannot be used.
    try:
    HTMLParser.__init__(self, convert_charrefs=False)
    except TypeError:
    # pre Python 3.3
    HTMLParser.__init__(self)
  • Somewhat redundant now:

    pelican/pelican/utils.py

    Lines 637 to 642 in bae6de5

    def escape_html(text, quote=True):
    """Escape '&', '<' and '>' to HTML-safe sequences.
    In Python 2 this uses cgi.escape and in Python 3 this uses html.escape. We
    wrap here to ensure the quote argument has an identical default."""
    return escape(text, quote=quote)
  • This looks a lot like os.makedirs with the new argument exist_ok=True:

    pelican/pelican/utils.py

    Lines 859 to 864 in bae6de5

    def mkdir_p(path):
    try:
    os.makedirs(path)
    except OSError as e:
    if e.errno != errno.EEXIST or not os.path.isdir(path):
    raise
  • Don't need the except part:
    try:
    from html import unescape # py3.5+
    except ImportError:
    from six.moves.html_parser import HTMLParser
    unescape = HTMLParser().unescape

PS: I didn't go through the tests yet, but I need a break :).

@avaris

This comment has been minimized.

Copy link
Member

avaris commented Nov 16, 2019

Bonus: I didn't verify but, I think all the magic in utils.pelican_open is handled by built-in open:

pelican/pelican/utils.py

Lines 256 to 266 in bae6de5

@contextmanager
def pelican_open(filename, mode='rb', strip_crs=(sys.platform == 'win32')):
"""Open a file and return its content"""
with codecs.open(filename, mode, encoding='utf-8') as infile:
content = infile.read()
if content[:1] == codecs.BOM_UTF8.decode('utf8'):
content = content[1:]
if strip_crs:
content = content.replace('\r\n', '\n')
yield content

@pauloxnet

This comment has been minimized.

Copy link
Contributor

pauloxnet commented Nov 17, 2019

I think all your comments are correct. Maybe you can push a commit with all the edits and we can see how tests say and then we can eventually comment the code.

@avaris

This comment has been minimized.

Copy link
Member

avaris commented Nov 17, 2019

I pushed the commits for the changes (+ a few more I noticed while doing them) I mentioned.

@avaris

This comment has been minimized.

Copy link
Member

avaris commented Nov 18, 2019

Added one more commit to clean up super() calls.

@iKevinY

This comment has been minimized.

Copy link
Member Author

iKevinY commented Nov 18, 2019

Thank you for adding to the PR @avaris and to @pauloxnet for the added review!

@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Nov 24, 2019

@avaris & @iKevinY: Do you think this PR is ready to merge? Just wanted to get your sign-off before merging. 馃槉

@iKevinY

This comment has been minimized.

Copy link
Member Author

iKevinY commented Nov 24, 2019

I'm good to merge; not sure if you'd like me to rebase first (to at least squash my commits together).

@avaris
avaris approved these changes Nov 24, 2019
Copy link
Member

avaris left a comment

Good for me

@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Nov 25, 2019

@iKevinY: Probably a good idea. Would you mind rebasing and squashing into logical commits?

iKevinY and others added 4 commits Nov 6, 2019
This commit removes Six as a dependency for Pelican, replacing the
relevant aliases with the proper Python 3 imports. It also removes
references to Python 2 logic that did not require Six.
@iKevinY

This comment has been minimized.

Copy link
Member Author

iKevinY commented Nov 26, 2019

@justinmayer Done, let me know if you'd like anything else changed!

@pauloxnet

This comment has been minimized.

Copy link
Contributor

pauloxnet commented Nov 26, 2019

Look good to me

@justinmayer

This comment has been minimized.

Copy link
Member

justinmayer commented Nov 26, 2019

Many thanks to @pauloxnet , @iKevinY , and @avaris for your work on this endeavor. Exciting to see the streamlined code! 馃挮

@justinmayer justinmayer merged commit 772005f into getpelican:master Nov 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@iKevinY iKevinY deleted the iKevinY:py2-sunset branch Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.