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

Add linting to GH action workflow #5

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

valefar-on-discord
Copy link
Collaborator

Include lint to GH action runner and ignore mypy issues for now.

I spent some time investigating but I believe each error is a result of an advanced inheritance model from py-ssz (https://github.com/ethereum/py-ssz/blob/main/ssz/sedes/byte_vector.py).

I wasn't able to find a reliable way to resolve these issues but I would rather ignore a few items than not have linting at all.

@valefar-on-discord
Copy link
Collaborator Author

I've been playing around trying to resolve the untyped mypy errors.
One method to resolve the specific error, of lets say Call to untyped function "ForkData" in typed context
You can utilize an init and call the super init with the params:

class ForkData(Serializable):
    fields = [
        ('current_version', bytes4),
        ('genesis_validators_root', bytes32),
    ]
    def __init__(self, current_version: bytes4, genesis_validators_root: bytes32) -> None:
        super().__init__(current_version, genesis_validators_root)

The problem here is this will create more errors:

error: Variable "ssz.sedes.byte_vector.bytes4" is not valid as a type  [valid-type]
note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
error: Variable "ssz.sedes.byte_vector.bytes32" is not valid as a type  [valid-type]
error: Call to untyped function "__init__" in typed context  [no-untyped-call]

I'm pretty sure there is a way to resolve the valid-type issue but I don't know how you would resolve the additional untyped-call.

Because of my struggle with this hydra, I decided to ignore them for now unless there are ideas on how to resolve them.

@remyroy
Copy link
Member

remyroy commented Apr 16, 2024

I've ran the ci jobs again and I don't see any error from running mypy. Here is an output I'm seeing from https://github.com/eth-educators/ethstaker-deposit-cli/actions/runs/8675308188/job/23881195586

Run python -m mypy --config-file mypy.ini -p staking_deposit
  python -m mypy --config-file mypy.ini -p staking_deposit
  shell: /usr/bin/bash -e {0}
  env:
    pythonLocation: /opt/hostedtoolcache/Python/3.1[2](https://github.com/eth-educators/ethstaker-deposit-cli/actions/runs/8675308188/job/23881195586#step:6:2).2/x64
    PKG_CONFIG_PATH: /opt/hostedtoolcache/Python/[3](https://github.com/eth-educators/ethstaker-deposit-cli/actions/runs/8675308188/job/23881195586#step:6:3).12.2/x64/lib/pkgconfig
    Python_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.2/x6[4](https://github.com/eth-educators/ethstaker-deposit-cli/actions/runs/8675308188/job/23881195586#step:6:4)
    Python2_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.2/x[6](https://github.com/eth-educators/ethstaker-deposit-cli/actions/runs/8675308188/job/23881195586#step:6:6)4
    Python3_ROOT_DIR: /opt/hostedtoolcache/Python/3.12.2/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.12.2/x64/lib
Success: no issues found in 64 source files

I'm guessing you fixed everything related to mypy.

@valefar-on-discord
Copy link
Collaborator Author

@remyroy I didn't fix them, I just ignored them...
signed_deposit = DepositData( # type: ignore[no-untyped-call]
as an example.

@remyroy
Copy link
Member

remyroy commented Apr 16, 2024

I see. Thanks for the precision.

Copy link
Member

@remyroy remyroy left a comment

Choose a reason for hiding this comment

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

Having these type check ignore is not ideal but it's better to run mypy regularly and have it pass on everything else than not having it or running it with errors and ignoring those errors.

pip install -r requirements_test.txt
- name: Run tests
run: pytest tests
- name: Run linter
Copy link
Member

Choose a reason for hiding this comment

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

mypy is not a linter. Having or adding a linter would be probably be great and it could be done in another PR.

I would rename this step to Run type checker

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

:)
ack

staking_deposit/credentials.py Show resolved Hide resolved
Copy link
Member

@remyroy remyroy left a comment

Choose a reason for hiding this comment

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

Everything is good.

@remyroy remyroy merged commit bce4b38 into eth-educators:main Apr 17, 2024
12 checks passed
@valefar-on-discord valefar-on-discord deleted the lint-workflow branch May 6, 2024 22:25
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