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

Replace more deprecated abstractproperty #7944

Merged
merged 1 commit into from
Dec 28, 2022
Merged

Replace more deprecated abstractproperty #7944

merged 1 commit into from
Dec 28, 2022

Conversation

Viicos
Copy link
Contributor

@Viicos Viicos commented Dec 28, 2022

Follow up on #7943 (for some reason, forgot to replace some of them).

As a side note (stated @jpadilla/pyjwt#843 (comment)), I think HashAlgorithm implementations should use the property decorator, e.g.:

class SHA256(HashAlgorithm):
    @property
    def name() -> str:
        return "sha256"
    ...

But imo another pattern should be used, as these classes are only storing data (perhaps dataclasses).

@reaperhulk
Copy link
Member

dataclasses were added in 3.7 so we can’t consider them until we drop 3.6 support (which we’re hoping to do as Ubuntu 18.04 drops off in the next 6 months). The mypy issues are unfortunate 😢

Copy link
Member

@reaperhulk reaperhulk left a comment

Choose a reason for hiding this comment

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

Incidentally the reason we left this with deprecated annotations originally was due to coverage issues with multiple decorators IIRC. Glad to see they seem to be resolved!

@reaperhulk reaperhulk merged commit 0a02a7d into pyca:main Dec 28, 2022
@Viicos
Copy link
Contributor Author

Viicos commented Dec 28, 2022

dataclasses were added in 3.7 so we can’t consider them until we drop 3.6 support (which we’re hoping to do as Ubuntu 18.04 drops off in the next 6 months). The mypy issues are unfortunate 😢

Ah yes forgot about that. The mypy issue can be fixed on the mentioned repo by using an instance of the hash algorithm instead of the class type. Still, is it valid to override properties the way it is done currently (that is overriding as a simple class attribute)? Apart from dataclasses, I was thinking about using ClassVar in the abc class:

class HashAlgorithm(metaclass=abc.ABCMeta):
    name: ClassVar[str]
    """
    A string naming this algorithm (e.g. "sha256", "md5").
    """
    digest_size: ClassVar[int]
    """
    The size of the resulting digest in bytes.
    """
    block_size: ClassVar[Optional[int]]
    """
    The internal block size of the hash function, or None if the hash
    function does not use blocks internally (e.g. SHA3).
    """

But then this would be breaking for hashes with a digest_size that can be initialized (e.g. SHAKE128). I guess it's better to wait for dataclasses support, and this is not a big issue anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants