Skip to content

Conversation

@15r10nk
Copy link

@15r10nk 15r10nk commented Nov 8, 2024

Change Summary

Implemented fallback if inline-snapshot is used with pypy (or any other none cpython implementation of python).
I hope to support pypy in the future but I don't know how long this will take.

Related issue number

fix #1534

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @davidhewitt

@codecov
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.36%. Comparing base (ab503cb) to head (4bc75c5).
Report is 224 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1533      +/-   ##
==========================================
- Coverage   90.21%   89.36%   -0.85%     
==========================================
  Files         106      112       +6     
  Lines       16339    17928    +1589     
  Branches       36       40       +4     
==========================================
+ Hits        14740    16022    +1282     
- Misses       1592     1886     +294     
- Partials        7       20      +13     

see 54 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2419981...4bc75c5. Read the comment docs.

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 8, 2024

CodSpeed Performance Report

Merging #1533 will not alter performance

Comparing 15r10nk:inline-snapshot-pypy-fix (4bc75c5) with main (2419981)

Summary

✅ 155 untouched benchmarks

@15r10nk
Copy link
Author

15r10nk commented Nov 10, 2024

please review.

Copy link
Contributor

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

This indeed makes tests pass on PyPy, thanks!

Comment on lines +3 to +5
if sys.implementation.name == 'cpython':
from inline_snapshot import snapshot
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to:

Suggested change
if sys.implementation.name == 'cpython':
from inline_snapshot import snapshot
else:
try:
from inline_snapshot import snapshot
except ImportError:

? I mean, I've packaged inline-snapshot for Gentoo CPython already, but I suppose it may save others trouble for the time being.

Copy link
Author

Choose a reason for hiding this comment

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

I got an idea. maybe we don't need this fix at all.
I can force inline-snapshot to run with --inline-snapshot=disable if something other then cpython is used. This would allow me to enable inline-snapshot again when I support it.

This would mean for you to package a inline-snapshot version for pypy which will not be able to create snapshots ... until I support it.

I think this would be better than to add a fix like this into every package which uses inline-snapshot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure — but could you make it possible to run a subset of inline-snapshot's test suite that covers the available functionality on PyPy then?

Copy link
Author

Choose a reason for hiding this comment

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

Yes I will write some extra tests. But the disable logic is already tested.

@15r10nk 15r10nk closed this Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inline-snapshot does not support pypy

3 participants