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

Introduce mypy type checking #17

Merged
merged 14 commits into from
Sep 13, 2021
Merged

Conversation

hukkin
Copy link
Contributor

@hukkin hukkin commented May 20, 2021

Closes #13

Fixes the make mypy and tox -e mypy invocations, which used to run unittests, now run mypy.
Fixes all mypy errors.

@popzxc popzxc requested a review from Deniallugo May 20, 2021 14:34
@hukkin hukkin changed the title Gradually introduce mypy type checking Introduce mypy type checking May 21, 2021
@hukkin
Copy link
Contributor Author

hukkin commented Jul 1, 2021

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.

@popzxc
Copy link

popzxc commented Jul 1, 2021

My 5 cents is that mypy is pretty important and it's better to have it. The final decision is up to @Deniallugo though.

@Deniallugo
Copy link
Collaborator

Sorry, for the big gap.
I think, that mypy is really important for this product.
So, please use it everywhere

@hukkin
Copy link
Contributor Author

hukkin commented Jul 2, 2021

@Deniallugo 👍 I fixed all the conflicts

fee_token: Token,
eth_auth_data: Union[ChangePubKeyCREATE2, ChangePubKeyEcdsa, None],
fee: int,
nonce: int = None,
Copy link
Contributor

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

Copy link
Collaborator

@vyastrebovvareger vyastrebovvareger Sep 10, 2021

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 ?

Copy link
Contributor

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

Copy link
Collaborator

@vyastrebovvareger vyastrebovvareger Sep 10, 2021

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Collaborator

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

Copy link
Contributor Author

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

@vyastrebovvareger
Copy link
Collaborator

vyastrebovvareger commented Sep 10, 2021

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: message = encode_defunct(message.encode()) must be type of SignableMessage

@hukkin
Copy link
Contributor Author

hukkin commented Sep 10, 2021

Fixed the mypy error. It's not a change in the API. It's just that eth_account didn't distribute type data before the latest version so mypy was not as strict here before.

@vyastrebovvareger
Copy link
Collaborator

I got the same issue with the test_is_public_key_onset. Currently I can't get the point why it's failed

@vyastrebovvareger
Copy link
Collaborator

Locally it's working fine

@vyastrebovvareger
Copy link
Collaborator

fail of test_is_public_key_onset occurs randomly but as far as I can see it depends on timeout and execution flow.

@StanislavBreadless StanislavBreadless merged commit a5452e9 into zksync-sdk:master Sep 13, 2021
@hukkin hukkin deleted the mypy branch September 16, 2021 13:04
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.

Missing tox [testenv] for mypy
7 participants