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

Fix deepcopy issue on Python3.5.0 #362

Closed

Conversation

macisamuele
Copy link
Collaborator

While working on Yelp/bravado#445, I've noticed that the deepcopy support added in #360 seems to not be working on Python3.5.0.

The goal of this PR is to fix the issue.

NOTE: This PR is still in draft as the issue is not solved, but as least we can reproduce it via travis

@coveralls
Copy link

coveralls commented Nov 29, 2019

Coverage Status

Coverage decreased (-0.1%) to 98.395% when pulling 59f0a36 on macisamuele:maci-run-tests-on-python-3.5.0 into 9c20a25 on Yelp:master.

@macisamuele
Copy link
Collaborator Author

I've dug a bit more into the test failures in Python 3.5.0 and I did found out that the issue is a known issue for the CPython implementation (issue link) and that it has already been resolved.

Unfortunately, the RefResolver of jsonschema library does use lru_cache into the class attributes and so this triggers the issue.

I've not found an easy/nice way to work this around but at the same time I do not expect that a lot of our users are using Python 3.5.0 (which should be the only affected version

My proposal at this point would be to update the bravado-core changelog highlighting the fact that deepcopy might not work properly on CPython 3.5.0.

Test run on all versions is available at: https://gist.github.com/macisamuele/edbdfee3bfdd10c33cc40ab654d9cb71

@macisamuele macisamuele marked this pull request as ready for review December 9, 2019 11:42
@@ -6,6 +6,8 @@ Changelog
-------------------
- Add equality and deepcopy support for Spec objects - `PR #360`_

**WARNING**: Deep Copy might not work properly on CPython 3.5.0. More details on `PR #362`_ and `Issue25447 <https://bugs.python.org/issue25447>`_
Copy link
Contributor

@sjaensch sjaensch Dec 10, 2019

Choose a reason for hiding this comment

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

I do not think this is a good pattern. If some part of our software is known to not work with an older Python version then either we need a workaround, disable that feature or discontinue support for that Python version.

I vote for the latter. There are three major Python releases since 3.5 out now, plus we don't need to drop support for all of Python 3.5, just 3.5.0. Other libraries do this as well, e.g. aiohttp requires Python 3.5.3+.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed.
Going to drop this PR in favour of #366

@macisamuele macisamuele closed this Feb 7, 2020
@macisamuele macisamuele deleted the maci-run-tests-on-python-3.5.0 branch February 7, 2020 15:44
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.

None yet

3 participants