-
Notifications
You must be signed in to change notification settings - Fork 926
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
Make classes inheriting from Type hashable #1944
Conversation
Thanks for the contribution. The change looks good to me, but can you add a a couple of simple unit tests for it so we can get it merged? |
I also had a quick look at the container Type implementation and it appears it doesn't define It seems like that for consistency reasons, An alternative could perhaps also be to completely remove those methods since it appear they are only present for backward compatibility reasons ( libcloud/libcloud/container/types.py Line 19 in abba8c1
|
Codecov Report
@@ Coverage Diff @@
## trunk #1944 +/- ##
=======================================
Coverage 83.18% 83.18%
=======================================
Files 352 352
Lines 81324 81324
Branches 8579 8579
=======================================
Hits 67647 67647
Misses 10873 10873
Partials 2804 2804
|
9bb0b21
to
e3d0720
Compare
e3d0720
to
39274c6
Compare
Done.
I'm very much in favour of doing that if we remove the dunder methods. |
Thanks.
To be on the safe side, I think it's better to do this in a separate add-on PR once this one is merged. |
The PR has been merged.
Once you get a chance, it would be great if you could make this change and also get rid of all the dunder methods which are there for backward compatibility reasons with older versions of Libcloud before Thanks. |
Make classes inheriting from Type hashable
Description
Attributes of classes that inherit from
libcloud.common.types.Type
are not hashable, so they can't be tested for membership in sets, unlike those that inherit fromlibcloud.container.types.Type
which work as intended.Status
done, ready for review.
Checklist (tick everything that applies)