Azure / msrest-for-python

The runtime library "msrest" for AutoRest generated Python clients.
MIT License
41 stars 64 forks source link

Add __hash__ to RecordSet to allow python3 hash key/set use #182

Closed ross closed 1 year ago

ross commented 5 years ago

Ran into this problem with the Azure DNS provider when updating octoDNS to support Python 3 in https://github.com/github/octodns/pull/384. It looks like objects that implement __eq__ in Python 3 are also required to implement a custom __hash__ method to match. RecordSet in azure-mgmt-dns==3.0.0 inherits from Model leading to:

(env3)otter:octodns ross$ ./script/coverage tests/test_octodns_provider_azuredns.py 2>&1 | tee /tmp/run.log
....E....
======================================================================
ERROR: test_populate_records (test_octodns_provider_azuredns.TestAzureDnsProvider)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ross/github/octodns/tests/test_octodns_provider_azuredns.py", line 449, in test_populate_records
    exists = provider.populate(zone)
  File "/Users/ross/github/octodns/octodns/provider/azuredns.py", line 386, in populate
    _records.add(azrecord)
TypeError: unhashable type: 'RecordSet'

I haven't dug around to see if there are other objects in msrest that need similar updates.

The actual hashing logic seems reasonable to me, but I'd welcome suggested improvements.

codecov-io commented 5 years ago

Codecov Report

Merging #182 into master will decrease coverage by 0.02%. The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
- Coverage   87.89%   87.86%   -0.03%     
==========================================
  Files          25       25              
  Lines        2593     2595       +2     
==========================================
+ Hits         2279     2280       +1     
- Misses        314      315       +1
Impacted Files Coverage Δ
msrest/serialization.py 91.03% <50%> (-0.09%) :arrow_down:

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 b505e36...5d8dd43. Read the comment docs.

ross commented 5 years ago

https://github.com/github/octodns/pull/384 added py3 support

ross commented 5 years ago

Nm. Closed one too many tabs worth. This still applies.

lmazuel commented 1 year ago

As this project is now deprecated, closing.