-
Notifications
You must be signed in to change notification settings - Fork 65
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
Introduce mypy type checking #17
Conversation
Hey @Deniallugo is mypy something that you want in this project? I'm asking because it took some time and effort to make it pass initially, and now after the latest changes there are pretty big merge conflicts (and mypy errors after fixing those), which I don't want to get into fixing if this isn't something you want merged. |
My 5 cents is that |
Sorry, for the big gap. |
@Deniallugo 👍 I fixed all the conflicts |
zksync_sdk/wallet.py
Outdated
fee_token: Token, | ||
eth_auth_data: Union[ChangePubKeyCREATE2, ChangePubKeyEcdsa, None], | ||
fee: int, | ||
nonce: int = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optional should've remained here, right? It seems like a part of the typing introduced by the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should work even without it. But yes, you are right. How can I checkout to this branch and fix it by the commit ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the best way is to start the new branch which preserves the history with @hukkin's commits and push it separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean checkout the : hukkin/zksync-python repo -> create the separated branch inside the master and do the new PR for that rigtht?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed mypy in the current branch for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit optional is not PEP484 compliant and mypy will error on it in the future: python/mypy#9091
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But It's not all. I'm doing the fixes now. there is only 1 question: Can i push into this repo separated branch, that's it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have write access to zksync-sdk/zksync-python you can also push to my branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There must be 1 more thing:
transfer, eth_signature = await self.build_forced_exit(target, token_obj
, fee_int
instead of just token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that's fixed in the commit I made
I do not know but it looks like the some library interface has been changed/updated. from eth_account.signers.base import BaseAccount now accepts only SignableMessage type. Let looking for fix Sorry. Looks strange because of: |
Fixed the mypy error. It's not a change in the API. It's just that |
I got the same issue with the test_is_public_key_onset. Currently I can't get the point why it's failed |
Locally it's working fine |
fail of |
Closes #13
Fixes the
make mypy
andtox -e mypy
invocations, which used to run unittests, now run mypy.Fixes all mypy errors.