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

Final, full protocol 2 support #82

Closed
wants to merge 6 commits into from

Conversation

marcintustin
Copy link
Contributor

This adds full pickle protocol 2 support:

  • __reduce__ and __reduce_ex__ may redirect to another object
  • Will use __reduce__ or __reduce_ex__ if provided, in preference to object __dict__ or __slots__; will not use __reduce__ or __reduce_ex__ for special-cased classes, and will not use the default object implementations of those methods.
  • Adds support for pickling iterators (and adds optional parameter max_iter to avoid livelocking on infinite iterators).
  • Adds support for __getinitargs__ and unpickling classic classes which take __init__ parameters, even if __getinitargs__ is not provided.

@davvid

@davvid
Copy link
Member

davvid commented Sep 2, 2014

This looks good. Please run git diff --check before committing; you can cleanup trailing whitespace by using git rebase --whitespace=fix ... which helps keeps things tidy. There's a few print statements in the tests that would be good to rebase squash out too.

Enable travis notifications on your repo; the tests seem to be failing under python3. ImportError: No module named __builtin__

There's also an isinstance(basestring, ...) part which might be better written as from jsonpickle.compat import unicode ... isinstance(x, (str, unicode)) to make it compatible across all python versions.

@marcintustin
Copy link
Contributor Author

@davvid All done, but I still have tests failing for 3.3. I'll figure that out and update you when it's ready.

@marcintustin
Copy link
Contributor Author

…port for __getinitargs__; this makes for full pickle protocol 2 support. Also has full PY3 compatibility.
@marcintustin
Copy link
Contributor Author

And here's the latest CI results for that last commit, which cleans up some comments: https://travis-ci.org/marcintustin/jsonpickle/builds/34204325

@marcintustin
Copy link
Contributor Author

This should close out #78

@davvid davvid closed this in 4850a16 Sep 2, 2014
@davvid
Copy link
Member

davvid commented Sep 2, 2014

Thanks Marcin, it's all merged now.

I rebased your branch so you might need to git reset --merge jsonpickle/master (assuming you've done git remote add jsonpickle git://github.com/jsonpickle/jsonpickle.git && git fetch jsonpickle) to get your repo in sync. I reworded a few of the commit messages so that they stay within 78 columns and did the --whitespace=fix thing to kill the trailing whitespace... these were small nits and were not worth holding up the merge ;-)

I also added a commit that eliminates the need for MAX_ITER since it looks like islice() does the right thing when None is passed in. That allowed us to get rid of that variable.

max_iter is still a generally useful feature so I exposed it in the encode() public API. That way users can specify a max_iter if needed.

Thanks a ton for making this happen! 👏

@marcintustin
Copy link
Contributor Author

@davvid Awesome, thanks for collaborating on that. I'm very pleased to be working on the project. I'm fairly certain this is now the most-pickling pickler for python 2 (in that it supports a proper superset of what python 2 ships with). With the Protocol 4 stuff I plan to add, it will be the most pickling pickler for python in general.

As to max_iter, I coded it to always be a finite limit, because infinite iterators are a normal case in python; that said, I understand the reasons why you'd want to leave it to the user to deal with that when it happens.

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

2 participants