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

Add type hints to expression tree #3497

Closed
pipliggins opened this issue Nov 3, 2023 · 1 comment · Fixed by #3578
Closed

Add type hints to expression tree #3497

pipliggins opened this issue Nov 3, 2023 · 1 comment · Fixed by #3578
Assignees
Labels
ongoing priority: medium To be resolved if time allows

Comments

@pipliggins
Copy link
Contributor

pipliggins commented Nov 3, 2023

Description

Type hints improve code readability and error detection, and make it easier (particularly for new contributors) to follow the structure & flow of data through large projects like PyBaMM.

Static type checkers such as MyPy/Pyright can be added at a later date (see #3489) and allow for gradual annotation of a codebase, so we don't need to add hints to the entire project at once.

As part of developing a solution to #3397 I added type hints to the bulk of the expression tree; this later turned out to be unneeded for serialisation but as they are useful more generally I figured they could be added via this issue instead.

@pipliggins
Copy link
Contributor Author

Moving/starting the discussion on the pros + cons of adding mypy/some form of static typing to here rather than on the PR (#3578 (comment)).

My general thought is that formally adding type hints to the codebase without some sort of check that this is being done consistently with existing hints risks making the code less, rather than more, readable (incorrect hints can send you down all sorts of rabbit holes). That said, using mypy to enforce type safety can quickly spiral into a cascade of issues that will need to be fixed, and in some cases making python code pass a static type checker can actually make the code less readable (although that may be down more to personal opinion).

My suggestion is to add mypy to the CI purely to provide informative messages to developers about potential mistakes in type hints, but not enforce it. Gradually adding typing to the codebase can be done slowly over time as various sections are edited.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ongoing priority: medium To be resolved if time allows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant