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

Remove Python 2 compatibility code #142

Open
4 tasks
medmunds opened this issue Oct 3, 2020 · 6 comments
Open
4 tasks

Remove Python 2 compatibility code #142

medmunds opened this issue Oct 3, 2020 · 6 comments

Comments

@medmunds
Copy link
Collaborator

medmunds commented Oct 3, 2020

Good first issue for anyone looking to contribute. Support for Python <3.5 was dropped after the 0.6.0 release, so there are some leftover Python 2-isms that can be removed from the code:

  • Drop object as a superclass—change all class ClassName(object): to just class ClassName:
  • Change all super(ClassName, self).method(...) calls to just super().method(...)
  • Remove all # -*- coding: utf-8 -*- markers (source and generated snapshot headers)
  • Remove all from __future__ imports (source and generated snapshot headers)

[Note: you might be tempted to also update string formatting to use f-strings, but those aren't available until Python 3.6, so we'll need to hold off for now.]

@paulmelnikow
Copy link
Collaborator

One of these is already done: remove from __future__ from the source. I'm all good on the changes that affect the sources.

Regarding the changes which affect the snapshots, I just wanted to think through for a minute:

  1. Am I correct that snapshots written in Python 3 (in e.g. 0.6.0) may not be compatible with Python 2?
  2. Will the changes you're outlining here cause unexpected diffs in people's snapshots after they upgrade?
  3. Do we want to provide a command people can run to clean up their snapshots to use the latest Python 3-only format?

@medmunds
Copy link
Collaborator Author

medmunds commented Oct 3, 2020

[Updated the from __future__ item in the list, and also removed the imp vs implib item, which on further investigation is probably not a good first issue.]

  1. Once (if) we remove the coding marker and unicode_literals import, snapshot files containing any non-ASCII characters will no longer be readable by Python 2. But if you've managed to install snapshottest > v0.6.0, you must already be on Python 3.5 or later. (There is a potential issue for teams somehow running mixed versions of snapshottest and/or Python. See Should verify snapshots came from supported snapshottest version #143.)

  2. The changes to the snapshot file header will cause diffs, but only in the header. (The actual snapshots have been written in Python 3 native format since v0.5.1, I think.) Probably worth a changelog note.

  3. The --snapshot-update option can be used to update all snapshots to the newest version.

Perhaps we should move updating the snapshot header into #143 or a new issue, and make this issue only about the source and test code?

@paulmelnikow
Copy link
Collaborator

Perhaps we should move updating the snapshot header into #143 or a new issue, and make this issue only about the source and test code?

That sounds like a great idea.

I could be wrong, though I’m thinking the header changes may not be a good first issue either.

@kam193
Copy link
Contributor

kam193 commented Nov 5, 2020

I will also recommend drop Python 3.5 support since dealing with some (now) strange limitations - like no support for path-like objects - is a bit frustrating ;)

@Kilo59
Copy link

Kilo59 commented Apr 11, 2024

@medmunds The use of imp seems to prevent snapshottest from working with python 3.12

File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/_pytest/assertion/rewrite.py", line 186, in exec_module
    exec(co, module.__dict__)
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/snapshottest/__init__.py", line 3, in <module>
    from .module import assert_match_snapshot
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/_pytest/assertion/rewrite.py", line 186, in exec_module
    exec(co, module.__dict__)
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/snapshottest/module.py", line 4, in <module>
    import imp
ModuleNotFoundError: No module named 'imp'

@medmunds
Copy link
Collaborator Author

@Kilo59 of course. I wasn't arguing to keep imp, I was just saying it was maybe not a "good first issue" like the other things on the list. Your PR looks helpful.

[Incidentally, I haven't been actively involved in this project for a few years now.]

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

No branches or pull requests

4 participants