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

Process: Have inputs property always return AttributesFrozenDict #6010

Merged
merged 1 commit into from
May 15, 2023

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented May 12, 2023

The Process.inputs property as implemented in plumpy has as a return type AttributesFrozenDict | None. This leads to a lot unnecessary complexities in the code having to deal with the potential None, where really this should never really occur. A lot of user code will never even check for Process.inputs returning None, such as in WorkChain implementations, and as a result type checkers will fail forcing a user to either unnecessarily complicate their code by explicitly checking for None, but will typically end up silencing the error.

The inputs property is overridden here to return an empty AttributesFrozenDict in case the inputs are None, which allows to simplify the return type and get rid of any type errors in downstream code.

The `Process.inputs` property as implemented in `plumpy` has as a return
type `AttributesFrozenDict | None`. This leads to a lot unnecessary
complexities in the code having to deal with the potential `None`, where
really this should never really occur. A lot of user code will never
even check for `Process.inputs` returning `None`, such as in `WorkChain`
implementations, and as a result type checkers will fail forcing a user
to either unnecessarily complicate their code by explicitly checking for
`None`, but will typically end up silencing the error.

The `inputs` property is overridden here to return an empty
`AttributesFrozenDict` in case the inputs are `None`, which allows to
simplify the return type and get rid of any type errors in downstream
code.
Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber. I didn't see any problem with this change, approved.

@sphuber sphuber merged commit 60756fe into aiidateam:main May 15, 2023
@sphuber sphuber deleted the fix/process-inputs branch May 15, 2023 20:26
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.

2 participants