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

Address Python 3.9 plistlib changes #74

Merged
merged 4 commits into from
Dec 5, 2020

Conversation

josh-hadley
Copy link
Collaborator

@josh-hadley josh-hadley commented Dec 4, 2020

  • drop support for Python < 3.6
  • don't use use_builtin_types in plistlib.loads
  • use bytes instead of plistlib.Data
  • code cleanup
  • add test case for line-wrapping
  • updates to TravisCI config

closes #73

- drop py2.7 support (setup.py)
- don't use `use_builtin_types` in `plistlib.loads`
- use `bytes` instead of `plistlib.Data`
Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

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

LGTM, but see comments

You also want to remove py27 from the .travis.yml config otherwise it'll keep failing. Bonus point if switching away from Travis to GH Actions

@@ -1293,7 +1294,7 @@ def _plistDate(self, data):
self.simpleElement("date", value=data)

def _plistData(self, data):
data = data.asBase64(maxlinelength=xmlTextMaxLineLength)
data = base64.b64encode(data)
Copy link
Member

Choose a reason for hiding this comment

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

here you're losing the line wrapping when length exceeds xmlTestMaxLineLength.
You should implement something like this and use it here:
https://github.com/python/cpython/blob/3689c25a1010c2acdb05f1b1b0229f4766b4440a/Lib/plistlib.py#L112-L119

Copy link
Member

Choose a reason for hiding this comment

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

and maybe also add a tests that exercises this line wrapping, because current tests didn't catch that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @anthrotype. I'll make some changes.

Since this is turning out to be a bit more than a quick fix: I think I'll spend some time and do a more thorough update, there are still some conditional imports to support Python 2.7, etc. that should also be removed. Related: I'm thinking about putting some stuff in that will require Python 3.6 minimum, seeing as how Python 3.5 support recently ended. Anyone have concerns about that? @typesupply @benkiel

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyone have concerns about that?

I don't have any concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

None either, remove away!

@josh-hadley josh-hadley marked this pull request as draft December 4, 2020 16:59
@anthrotype
Copy link
Member

Python 3.5 is EOL, it can be safely dropped by now

- add flake8 install
- remove Python 2.7
- add Python 3.7, 3.8, and 3.9
- add flake8 to script
- cleaned up many, many flake8 issues (formatting, unused imports, line lengths, ...)
- fixed some additional issues such as mutable data structures for arg defaults
- purged code used to support Python < 3.6
- implement _encode_base64 (and _decode_base64) per review
- added test to check text wrapping
@josh-hadley josh-hadley marked this pull request as ready for review December 4, 2020 22:52
@josh-hadley
Copy link
Collaborator Author

@anthrotype @benkiel and anyone else who wants to chime in: I made the changes requested (and more). The Travis check on Python 3.8 is failing in a very odd way that I don't really understand. I can't repro it here, some weird glitch. I think it's okay so hoping you can overlook that in your review. Related: Ben has given me admin access, which I'll use to implement GitHub Actions and ditch TravisCI once this PR is sorted.

@benkiel
Copy link
Contributor

benkiel commented Dec 5, 2020

@josh-hadley this looks good to me, and yes that Python 3.8 error is super weird, will overlook. Going to merge this, once GhitHub actions is in place we can cut a release. Thank you so much for taking care of this.

@benkiel benkiel merged commit baa9560 into unified-font-object:master Dec 5, 2020
@josh-hadley josh-hadley deleted the jh-use-data branch December 5, 2020 19:31
@josh-hadley
Copy link
Collaborator Author

Sure thing, @benkiel . If someone from the unified-font-object org can add org- or repo-level secrets with the PyPI token (as e.g. PYPI_PASSWORD) I can set up a few GitHub Action workflows for testing, making a release (including publishing to PyPI). I've set this up for our AFDKO and psautohint repos and it's super-handy; basically all you have to do to make a release is push a tag with the version (will need to make a few minor tweaks to the code to handle scm versioning).

@benkiel
Copy link
Contributor

benkiel commented Dec 5, 2020

@josh-hadley done!

ammgws added a commit to ammgws/ufoNormalizer that referenced this pull request Dec 6, 2020
To reflect changes made in unified-font-object#74
@ammgws ammgws mentioned this pull request Dec 6, 2020
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.

plistlib changed in Python 3.9: use_builtin_types removed
4 participants