Add __eq__ to HashAlgorithm and padding instances

[mathiasertl]

This PR:

1. Adds __eq__() to all subclasses of HashAlgorithm.
2. Adds __eq__() to all padding classes.

My primary use case is testing. First, it's really un-pythonic to me (obviously very subjective, but it feels just not right to me) to write assert isinstance(cert.cert.signature_hash_algorithm, hashes.SHA256()), when I could write assert cert.cert.signature_hash_algorithm == hashes.SHA256(). But once you start comparing to other data structures, it gets even uglier:

def test_something() -> None:
    ...
    assert isinstance(cert.signature_hash_algorithm, type(other.signature_hash_algorithm))

I think this just looks a lot better and readable:

def test_something() -> None:
    ...
    assert cert.signature_hash_algorithm == other.signature_hash_algorithm

[reaperhulk]

Let's put these in a separate PR. Whether or not we want to do __eq__ is possibly debateable, but if you want these properties to be public that's a separate discussion.

[mathiasertl]

Hi @reaperhulk

Thanks for the feedback - I've updated this PR to only include the __eq__ instances. I'll make a separate PR for the extra properties.

kr, Mat

[reaperhulk]

Looking at the test failures here the issue is that scapy creates its own HashAlgorithm, which now fails because they don't implement __eq__. We could avoid this by making __eq__ a concrete method that raises NotImplementedError, but we'd need to cover those lines in a test as well.

[mathiasertl]

Or it could be a breaking change and that lib would need to implement the method.

Open to implement any solution, just let me know what you prefer!

[reaperhulk]

@alex do you have an opinion here? I'm reluctant to spend breakage budget on this when we could avoid it with a concrete implementation, but I'm interested in other opinions.

[alex]

I'm pretty ambivalent about this. We have a few choices:

1. Simply implement __eq__ for our own hashes, don't change the contract (all objects always have __eq__).
2. Add __eq__ to the ABC and require people to implement it: breaking change for people defining their own hashes
3. Add __eq__ to the ABC and provide a default impl: works for some users with their own hashes, but is wrong for anyone with their own hash which has any attributes (e.g. blake2).

Of these I think (1) is probably best.

[reaperhulk]

For 3 the default impl could raise NotImplementedError so you wouldn’t need to have a potentially incorrect impl

[alex]

That can break existing applications, to the extent they're relying on the default object.__eq__ behavior!

[mathiasertl]

I'm also ambivalent, I defer to your call and will adapt as requested.

Just option three (default Implementation) seems like a bad idea to me.

Mat

[reaperhulk]

After thinking for too long, I'm okay with option 1 here.