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 annotations for python3.6 and below #197

Merged
merged 3 commits into from
Sep 19, 2018

Conversation

cicdw
Copy link
Contributor

@cicdw cicdw commented Sep 6, 2018

Closes #193 and closes #196 with a heavy hand, by removing the ability to include __annotations__ in versions of python < 3.7.

It seemed the ability to pickle arbitrary typing classes was a deep rabbit hole external to cloudpickle, and AFAIK the alternative would be some sort of check / try / except on annotations that would lead to inconsistent pickling of __annotations__.

@ogrisel
Copy link
Contributor

ogrisel commented Sep 14, 2018

I have no strong opinion. I believe it's ok to remove a half broken half working feature. We should probably do it as part of 0.6 and document that well in the changelog.

@ogrisel
Copy link
Contributor

ogrisel commented Sep 19, 2018

I pinged several reviewers to get a second opinion on this change.

@pitrou
Copy link
Member

pitrou commented Sep 19, 2018

Agreed with this. It's unfortunate that typing was designed in such a baroque and unusual way. Not the greatest Python feature IMO :-/

@mrocklin
Copy link
Contributor

No objection from me. I don't know this topic well though. I'm inclined to trust @pitrou .

@ogrisel
Copy link
Contributor

ogrisel commented Sep 19, 2018

@cicdw can you please add an entry to document this change to the change log file?

@cicdw
Copy link
Contributor Author

cicdw commented Sep 19, 2018

@ogrisel yup, done.

@codecov-io
Copy link

codecov-io commented Sep 19, 2018

Codecov Report

Merging #197 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #197   +/-   ##
=======================================
  Coverage   85.16%   85.16%           
=======================================
  Files           1        1           
  Lines         573      573           
  Branches      111      111           
=======================================
  Hits          488      488           
  Misses         62       62           
  Partials       23       23
Impacted Files Coverage Δ
cloudpickle/cloudpickle.py 85.16% <100%> (ø) ⬆️

Continue to review full report at Codecov.

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

@ogrisel
Copy link
Contributor

ogrisel commented Sep 19, 2018

Alright, let's merge then! Thanks @cicdw.

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.

typing.Generic-based type annotations cannot be loaded Pickling Annotations can fail
5 participants