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

change linting to use pre-commit #3297

Merged
merged 15 commits into from Apr 10, 2024
Merged

change linting to use pre-commit #3297

merged 15 commits into from Apr 10, 2024

Conversation

pacrob
Copy link
Contributor

@pacrob pacrob commented Mar 20, 2024

What was wrong?

pre-commit is totally rad and we should use it for web3 v7!

How was it fixed?

Added pre-commit, removed previous linting setup, moved config to pyproject.toml where possible.

To aid in review, I've made one commit for each new lint tool. It's still a big pr but should be more manageable that way.

Todo:

Cute Animal Picture

image

@pacrob pacrob force-pushed the add-pre-commit branch 13 times, most recently from fb4daeb to 235871f Compare March 25, 2024 20:44
@pacrob pacrob marked this pull request as ready for review March 25, 2024 20:45
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

Nice improvements! lgtm

Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

Had a few comments. I don't think turning off noqa B008 on whole files is ideal. Also, should we ignore large json spec files for ENS? I don't see a benefit in formatting those but could go either way. I don't see a specific json formatter in pre-commit, just yaml and toml.

According to the github merge check there's a conflict too that likely needs to be resolved before merging.

web3/providers/rpc/async_rpc.py Outdated Show resolved Hide resolved
@@ -63,7 +63,7 @@ def __init__(
request_kwargs: Optional[Any] = None,
session: Optional[Any] = None,
exception_retry_configuration: Optional[ExceptionRetryConfiguration] = (
ExceptionRetryConfiguration(
ExceptionRetryConfiguration( # noqa: B008
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same thing as the async_rpc.py case here. This feels like a valid B008 case. I think we should set it to None here and below:

        self.exception_retry_configuration = (
            exception_retry_configuration
            or ExceptionRetryConfiguration(
                errors=(
                    ConnectionError,
                    requests.HTTPError,
                    requests.Timeout,
                )
            )
        )

ens/async_ens.py Outdated
@@ -1,3 +1,4 @@
# flake8: noqa: B008
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should ignore whole files like this unless we have good reason to. We can resolve the three specific issues by setting arguments to None and addressing them in the method with if x is None: {{set the default value}}.

This guarantees it's not just set once. Even if that's intended behavior, we should add noqa: B008 to single lines and explain why they're there with a comment instead of whole files. That would make it easy to mask future errors since it's sneakily up at the top of the file.

Specifically, in this file, we can address:

  • provider in __init__()
  • address in setup_address()
  • new_owner in setup_owner().

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 put those there instead of inline because I made a note "talk to Felipe about the B008 ignores". And then forgot about it.

ens/ens.py Outdated Show resolved Hide resolved
ens/utils.py Outdated Show resolved Hide resolved
web3/main.py Outdated Show resolved Hide resolved
ens/specs/nf.json Show resolved Hide resolved
@pacrob pacrob force-pushed the add-pre-commit branch 8 times, most recently from b871c90 to c9d5284 Compare March 27, 2024 17:42
@pacrob
Copy link
Contributor Author

pacrob commented Mar 27, 2024

I've cleaned up per comments. All added casts have been removed as unneeded due to setting mypy's warn_return_any = false.

There are 4 remaining B008 rule ignores. 2 are related to ENS. Fails these tests:
tests/ens/test_setup*.py

The other 2 are related to HTTP provider retry config. Removing them fails:
tests/core/providers/test_http_request_retry.py

I tried a few things to fix them but no dice. Let me know if you'd like to try to fix them here or open a new issue to deal with them separately.

@pacrob pacrob requested a review from fselmo March 27, 2024 18:14
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

That is a terrifying cute animal picture, lol. I think the typing changes will be breaking, so I think that this can only go into v7. I may be thinking about that wrong though, so let me know if I am. Thanks for getting this done, it will be so nice to have precommit in web3 finally! The commits made it super easy(/manageable :) ) to review, thank you!

web3/_utils/request.py Show resolved Hide resolved
web3/tools/benchmark/main.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
docs/README-windows.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for clearing up those comments! 🚀

Copy link
Collaborator

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

Looks good! This will be amazing to get in, ty.

@pacrob pacrob merged commit a9de8c8 into ethereum:main Apr 10, 2024
81 checks passed
@pacrob pacrob deleted the add-pre-commit branch April 10, 2024 22:32
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.

None yet

4 participants