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

👌 IMPROVE: Add full typing, type checking and test coverage #180

Merged
merged 19 commits into from
Jan 5, 2021

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Nov 21, 2020

Hey guys, as the title suggests!

I've also dropped python 3.5 support (as we have also done in aiida-core), added a tox testing configuration, added codecov reporting, and updated the documentation build/ci.

All the tests should pass, except I have left some # TODOs regarding some type clarifications and possible errors detected:

plumpy/communications.py:29:2: W0511: TODO identifier type? (fixme)
plumpy/communications.py:146:2: W0511: TODO type of task and msg; dict? (fixme)
plumpy/ports.py:306:2: W0511: TODO this was set to None, but that would fail if you tried to compute breadcrumbs (fixme)
plumpy/utils.py:72:2: W0511: TODO I deleted ListenContext and ThreadSafeCounter, (fixme)
plumpy/__init__.py:22:2: W0511: TODO I deleted stack.py, because is not used or tested any where in this code base, or in aiida-core (fixme)
plumpy/process_comms.py:31:2: W0511: TODO process result/status type? (fixme)
plumpy/workchains.py:387:2: W0511: TODO this was added (fixme)
plumpy/process_states.py:253:2: W0511: TODO error: "Kill" has no attribute "result"/"successful" (fixme)
plumpy/base/state_machine.py:250:2: W0511: TODO should class initialise sealed = False? (fixme)
plumpy/base/state_machine.py:257:2: W0511: TODO this appears to never be used? (fixme)
plumpy/persistence.py:646:2: W0511: TODO according to types self._callbacks should first be called, i.e. self._callbacks() (fixme)

over to you 😬

@chrisjsewell chrisjsewell changed the title Add type checking Add full typing and type checking Nov 21, 2020
@chrisjsewell chrisjsewell mentioned this pull request Nov 21, 2020
muhrin
muhrin previously approved these changes Nov 21, 2020
Copy link
Collaborator

@muhrin muhrin left a comment

Choose a reason for hiding this comment

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

Thanks Chris! Looks good to me, let see if the others are happy before merging.

@codecov-io
Copy link

codecov-io commented Nov 21, 2020

Codecov Report

❗ No coverage uploaded for pull request base (develop@2c0b029). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop     #180   +/-   ##
==========================================
  Coverage           ?   90.94%           
==========================================
  Files              ?       22           
  Lines              ?     2922           
  Branches           ?        0           
==========================================
  Hits               ?     2657           
  Misses             ?      265           
  Partials           ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c0b029...8361ccb. Read the comment docs.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented Nov 21, 2020

plumpy/process_states.py:253:2: W0511: TODO error: "Kill" has no attribute "result"/"successful" (fixme)

This is not covered by testing, so is likely to be a legitimate bug (potentially related to aiidateam/aiida-core#3888)

@chrisjsewell chrisjsewell changed the title Add full typing and type checking Add full typing, type checking and test coverage Nov 21, 2020
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 @chrisjsewell, this really a helpful feature to have type hint. Just some minor request.

Out of curious, when we calling methods with the wrong type, what information will be given to the user? Is type hint only a hint or it really do the type check?

.github/workflows/ci.yml Show resolved Hide resolved
plumpy/__init__.py Outdated Show resolved Hide resolved
plumpy/communications.py Show resolved Hide resolved
plumpy/communications.py Outdated Show resolved Hide resolved
plumpy/process_comms.py Outdated Show resolved Hide resolved
plumpy/__init__.py Outdated Show resolved Hide resolved
plumpy/processes.py Outdated Show resolved Hide resolved
plumpy/__init__.py Outdated Show resolved Hide resolved
unkcpz
unkcpz previously approved these changes Dec 11, 2020
@chrisjsewell
Copy link
Member Author

Thanks @chrisjsewell, this really a helpful feature to have type hint. Just some minor request.

right this all works now, and requests have been addressed, so I will be merging very shortly; so speak now or for ever hold your peace 😉

Out of curious, when we calling methods with the wrong type, what information will be given to the user? Is type hint only a hint or it really do the type check?

type hinting is not used at all at runtime, it is effectively stripped from the code.
But it allows for better auto-completion in editors and for dependant packages to type check against classes/functions used from this package.

Once this is released, I can add proper type hinting/checking to the aiida-core engine code, which I think will be very useful for users/plugin developers writing workflows etc

@chrisjsewell chrisjsewell requested a review from unkcpz January 5, 2021 00:20
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! all looks good to me.

@chrisjsewell chrisjsewell changed the title Add full typing, type checking and test coverage 👌 IMPROVE: Add full typing, type checking and test coverage Jan 5, 2021
@chrisjsewell chrisjsewell merged commit 16e7713 into develop Jan 5, 2021
@chrisjsewell chrisjsewell deleted the mypy branch January 5, 2021 14:41
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.

4 participants