Skip to content

Remove six library dependency#93

Open
maxwellyoung wants to merge 1 commit into
data-govt-nz:masterfrom
maxwellyoung:remove-six-library
Open

Remove six library dependency#93
maxwellyoung wants to merge 1 commit into
data-govt-nz:masterfrom
maxwellyoung:remove-six-library

Conversation

@maxwellyoung
Copy link
Copy Markdown

Summary

Removes the six library (Python 2/3 compatibility shim) as it is no longer needed. Python 2 reached end-of-life in January 2020, and CKAN has dropped Python 2 support.

Closes #88

Changes

  • validators.py: Replace six.string_types with str, six.text_type() with str()
  • mailer.py: Replace six.ensure_text() with bytes.decode('ascii')
  • schema.py: Remove unused import six
  • requirements.txt: Remove six~=1.16.0

Testing

All replacements are direct equivalents in Python 3:

  • six.string_typesstr (in Python 3, six.string_types is just (str,))
  • six.text_typestr (in Python 3, six.text_type is str)
  • six.ensure_text(bytes_val)bytes_val.decode('ascii') (make_key() returns hex-encoded bytes, always ASCII-safe)

Replace all six usage with native Python 3 equivalents:
- six.string_types -> str
- six.text_type -> str
- six.ensure_text -> bytes.decode()
- Remove six from requirements.txt

Closes data-govt-nz#88
@ThrawnCA
Copy link
Copy Markdown
Contributor

IIUC ckanext-security still supports CKAN 2.9

@maxwellyoung
Copy link
Copy Markdown
Author

Good point — yes, ckanext-security still supports CKAN 2.9. However, CKAN 2.9 requires Python 3 (Python 2 support was dropped), and in Python 3 all of the six shims resolve to their native equivalents:

  • six.string_types(str,)
  • six.text_typestr
  • six.ensure_text(s) → returns s unchanged if already str, or s.decode('utf-8') if bytes

Since this extension dropped pre-2.9 support (and therefore Python 2) from tag 4.0.0, six is effectively a no-op dependency. The replacements in this PR are functionally identical on Python 3.

The one nuance is six.ensure_text(make_key())make_key() returns bytes (from codecs.encode), and six.ensure_text would decode with utf-8. I used .decode('ascii') since hex-encoded bytes are always ASCII-safe, but happy to change to .decode('utf-8') if you'd prefer consistency.

@ThrawnCA
Copy link
Copy Markdown
Contributor

However, CKAN 2.9 requires Python 3 (Python 2 support was dropped)

Which 2.9? My recollection is that 2.9 could handle either Python 2 or 3.

@maxwellyoung
Copy link
Copy Markdown
Author

You're right — CKAN 2.9 itself supports both Python 2 and Python 3. The README says "CKAN 2.9.x and Python 3 support was added from git tag 3.0.0" but doesn't explicitly state that Python 2 is no longer supported.

That said, looking at the codebase on the master branch (post-4.0.0), there are no Python 2 compatibility patterns outside of six itself — f-strings aren't used but there's also no from __future__ import statements, and the code uses Python 3 idioms throughout.

If the maintainers want to keep Python 2 support for CKAN 2.9 users, then six should stay and this PR can be closed. If Python 2 support has been effectively dropped (even if not explicitly documented), then this cleanup is appropriate.

Happy to defer to @markstuart on the intended support matrix.

@markstuart
Copy link
Copy Markdown
Contributor

Thanks for this @maxwellyoung, and thanks for the push for clarity from @ThrawnCA as well.

I think it makes the most sense to draw a line in the sand at this point and say that once this PR is tested and goes in, we won't support Python 2 anymore. I can't imagine many scenarios where CKAN site maintainers wouldn't be able to at least upgrade to a CKAN version that supports Python 3, and if they really can't, they are probably already using an old pinned version of this repo.

My plan is to get the 2.11 support branch merged with your docs update PR @maxwellyoung (thanks again), push a tag that will represent the final Python 2 support tag, then merge this PR and update the README to clarify that Python 2 support ended at that tag.

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.

remove six library

3 participants