syrusakbary / snapshottest

Snapshot Testing utils for Python 📸
MIT License
525 stars 102 forks source link

Remove Python 2 compatibility code #142

Open medmunds opened 3 years ago

medmunds commented 3 years ago

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:

[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 commented 3 years ago

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 commented 3 years ago

[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 #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 commented 3 years ago

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 commented 3 years ago

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 commented 3 months ago

@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 commented 3 months ago

@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.]