Skip to content

Improve bip32.py type hint #107

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

Merged
merged 8 commits into from
Oct 25, 2022

Conversation

daehan-koreapool
Copy link
Contributor

Since I believe PyCardano will be a foundational and critical component of the future Python Cardano dev ecosystem, I would like to start improving PyCardano code quality by improving type hint one bit at a time.

Goal

  • improve bip32.py module's type hint enforcement
  • possibly include mypy in the ci/cd pipeline without failure
$ poetry run mypy pycardano/crypto/bip32.py
pycardano/crypto/bip32.py:134: error: Incompatible types in assignment (expression has type "bytearray", variable has type "str")
pycardano/crypto/bip32.py:135: error: Argument 1 to "_tweak_bits" of "HDWallet" has incompatible type "str"; expected "bytearray"
pycardano/crypto/bip32.py:335: error: Value of type "Optional[bytes]" is not indexable
pycardano/crypto/bip32.py:336: error: Value of type "Optional[bytes]" is not indexable
pycardano/crypto/bip32.py:344: error: Incompatible types in assignment (expression has type "Tuple[Optional[bytes], Optional[bytes], str]", variable has type "Tuple[Union[bytes, Any], Union[bytes, Any], Optional[bytes], Optional[bytes], str]")
pycardano/crypto/bip32.py:354: error: Syntax error in type annotation
pycardano/crypto/bip32.py:354: note: Suggestion: Use Tuple[T1, ..., Tn] instead of (T1, ..., Tn)
pycardano/crypto/bip32.py:396: error: Incompatible return value type (got "None", expected "HDWallet")
pycardano/crypto/bip32.py:444: error: Syntax error in type annotation
pycardano/crypto/bip32.py:444: note: Suggestion: Use Tuple[T1, ..., Tn] instead of (T1, ..., Tn)
pycardano/crypto/bip32.py:458: error: Incompatible return value type (got "None", expected "HDWallet")
pycardano/crypto/bip32.py:553: error: Missing return statement
Found 10 errors in 1 file (checked 1 source file)

@daehan-koreapool daehan-koreapool changed the title ADD. adding mypy to dev-dependency WIP: Improve bip32.py type hint Oct 25, 2022
@daehan-koreapool daehan-koreapool marked this pull request as draft October 25, 2022 05:14
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2022

Codecov Report

Base: 86.15% // Head: 86.17% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (5427cfb) compared to base (f054d47).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
+ Coverage   86.15%   86.17%   +0.01%     
==========================================
  Files          24       24              
  Lines        2630     2625       -5     
  Branches      517      515       -2     
==========================================
- Hits         2266     2262       -4     
- Misses        268      269       +1     
+ Partials       96       94       -2     
Impacted Files Coverage Δ
pycardano/crypto/bip32.py 91.62% <100.00%> (+0.31%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@daehan-koreapool
Copy link
Contributor Author

Major Updates

  • Made most of the HDWallet constructor parameters non-optional typed since they are all used in each derivation step
  • Identified there are no such cases where public_pnode and private_pnode can ever be empty

Minor Updates

  • Avoid type converting already defined variable
  • Fixed type hint annotation syntax

@daehan-koreapool daehan-koreapool changed the title WIP: Improve bip32.py type hint Improve bip32.py type hint Oct 25, 2022
@daehan-koreapool daehan-koreapool marked this pull request as ready for review October 25, 2022 06:47
@cffls
Copy link
Collaborator

cffls commented Oct 25, 2022

This is great, thanks for creating the first mypy check! One suggestion, could you also change this CI action to use mypy: https://github.com/cffls/pycardano/blob/main/.github/workflows/main.yml#L33? I think make qa is sufficient.

ADD. adding a mypy exclude list which will act like a todo list in the future
@daehan-koreapool
Copy link
Contributor Author

This is great, thanks for creating the first mypy check! One suggestion, could you also change this CI action to use mypy: https://github.com/cffls/pycardano/blob/main/.github/workflows/main.yml#L33? I think make qa is sufficient.

Hi Jerry,

I've added a list of python modules to be excluded in the mypy and generalized make qa to target the entire pycardano package rather than a single bip32.py module.
By doing so, any new modules added to the pycardano repo will need to pass mypy check, and we will be able to tackle one module at a time to improve type hint.

It seems there was an error on Codecov service while running the ci/cd pipeline.
I'm more familiar to Gitlab pipeline UX. Do you know how to re-trigger the pipeline step on Github?

@cffls
Copy link
Collaborator

cffls commented Oct 25, 2022

I've added a list of python modules to be excluded in the mypy and generalized make qa to target the entire pycardano package rather than a single bip32.py module.
By doing so, any new modules added to the pycardano repo will need to pass mypy check, and we will be able to tackle one module at a time to improve type hint.

Awesome, thanks for creating this smart check!

@cffls cffls merged commit e0540e3 into Python-Cardano:main Oct 25, 2022
@cffls
Copy link
Collaborator

cffls commented Oct 25, 2022

Oops, missed your last question, sorry. You can go click on the failed job, and in the upper-right corner of the workflow, use the Re-run jobs drop-down menu, and select Re-run all jobs. See this doc: https://docs.github.com/en/actions/managing-workflow-runs/re-running-workflows-and-jobs

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