search

openwallet-foundation / acapy

Hyperledger Aries Cloud Agent Python (ACA-Py) is a foundation for building decentralized identity applications and services running in non-mobile environments.
https://wiki.hyperledger.org/display/aries
Apache License 2.0
408 stars 512 forks source link

:art: fix type hints for optional method parameters #3234

Closed ff137 closed 3 weeks ago

ff137 commented 3 weeks ago

This PR updates the type hints for optional method parameters across the codebase. The previous pattern param: str = None has been replaced with param: Optional[str] = None for improved clarity and type checking.

The pattern of saying param: str = None is a bit of a code smell, since we're telling the compiler it is a string, when it can be (and is by default) None.

Usually not a problem, but type checkers can mistakenly indicate code to be unreachable:

image

^ Note that the last line is greyed out. It has message:

image

This is because we've given the type hint EndpointType, and the compiler thinks that can never be "falsey". To fix, we should tell the compiler it is an Optional[EndpointType].

Apart from type checking, it just makes the code more clear and self-explanatory.

So, I used regex to replace (\w+): (\w+) = None with $1: Optional[$2] = None, and add import to modules that need it. Separately replaced List[..] = None types. Only applied changes to aries_cloudagent folder, not demo.

sonarcloud[bot] commented 3 weeks ago

Quality Gate Failed Quality Gate failed

Failed conditions
4.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

dbluhm commented 3 weeks ago

This has been on the back of my mind for a long while. Thanks for taking care of this! I think there will be some minor repercussions; there are at least a few manager classes that generally expect attributes it's accessing to not be None despite the init parameters of the object in question having param: X = None. These will of course only matter during static analysis/type checking. These corrected type hints are also more true to the actual usage of the object. Given that, I think it's still the right choice to make these corrections.