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

🎨 fix type hints for optional method parameters #3234

Merged

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Sep 16, 2024

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.

Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

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

This looks like a good PR to me. Will tag @dbluhm for any comments he has.

@jamshale jamshale requested a review from dbluhm September 16, 2024 16:47
@ff137 ff137 force-pushed the art/fix-bad-unreachable-code-warning branch from ecf626c to ae776a8 Compare September 16, 2024 18:11
@ff137 ff137 force-pushed the art/fix-bad-unreachable-code-warning branch from ae776a8 to f1b7708 Compare September 16, 2024 18:46
Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@dbluhm
Copy link
Contributor

dbluhm commented Sep 16, 2024

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.

Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks again! I think we can also safely ignore the code duplication warning from Sonar, too, of course.

@dbluhm dbluhm merged commit 17e9f16 into openwallet-foundation:main Sep 16, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants