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

Add support for abc.abstract* methods #450

Merged
merged 3 commits into from Nov 29, 2021

Conversation

jrbourbeau
Copy link
Member

This PR adds support for abc.abstractproperty, abc.abstractclassmethod, and abc.abstractstaticmethod. The changes here are mostly a duplicate of what was proposed in #369 by @KristianHolsheimer plus a copy of the tests added in #371, but using the abc.abstract* methods. Additionally, this PR extends test_abc a bit to include coverage for abstract @propertys.

As pointed out in #367 (comment), the abc.abstract* methods added here are now deprecated, but it seems reasonable to add support for them anyways as users will still run into them from time to time (xref #394)

Closes #367

@codecov
Copy link

codecov bot commented Sep 25, 2021

Codecov Report

Merging #450 (2f97c6d) into master (6099fdb) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #450      +/-   ##
==========================================
+ Coverage   92.52%   92.56%   +0.04%     
==========================================
  Files           4        4              
  Lines         709      713       +4     
  Branches      151      155       +4     
==========================================
+ Hits          656      660       +4     
  Misses         32       32              
  Partials       21       21              
Impacted Files Coverage Δ
cloudpickle/cloudpickle_fast.py 96.91% <100.00%> (+0.03%) ⬆️

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 6099fdb...2f97c6d. Read the comment docs.

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM. Could you please document the change in the changelog?

@KristianHolsheimer
Copy link

Thanks for fixing this @jrbourbeau

@jrbourbeau
Copy link
Member Author

Thanks for reviewing @ogrisel @KristianHolsheimer -- just pushed up a changelog entry

@jrbourbeau
Copy link
Member Author

Just checking in here, @ogrisel is there anything else that should be added to this PR?

@jakirkham
Copy link
Member

Sounds like the changelog entry was all that was needed, which has since been added. So going to go ahead and merge. Though please let us know if we missed anything else and we can follow up :)

@jakirkham jakirkham merged commit 5d89947 into cloudpipe:master Nov 29, 2021
@ogrisel
Copy link
Contributor

ogrisel commented Nov 29, 2021

Sorry for the lack of reply. Too many github notifications. Thanks for the PR @jrbourbeau and merging it @jakirkham.

@jakirkham
Copy link
Member

No worries. Know the feeling ;)

@jrbourbeau jrbourbeau deleted the abstracts branch November 29, 2021 21:27
@jrbourbeau
Copy link
Member Author

Yeah, no worries at all -- you're in good company : )

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.

Cannot pickle classes derived from ABC with abc.abstract* decorators
4 participants