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

[2/3] Linted file with isort in eth and tests directory #2099

Merged

Conversation

prathmeshranaut
Copy link
Contributor

@prathmeshranaut prathmeshranaut commented Apr 19, 2023

What was wrong?

Imports are not sorted.

Closes Issue #2094

How was it fixed?

Added isort, a tool to sort imports, in the linting process.

Adding lint setup + linting files would be a large PR to review. I've decided to split it up into 3 parts.
First PR

flowchart LR
    pr1[Added isort to setup, makefile and tox] --> pr2[Linted file with isort in eth and tests directory - This PR]
    pr2 --> pr3[Add isort check to tox]
Loading

This is second PR of a two part linting rollout. In this PR, I ran make lint-roll.
In order to validate and review the PR quickly, you can checksum eth and tests directory.

Let me know if I missed anything in the contribution process.

Todo:

  • Clean up commit history

  • Add or update documentation related to these changes

  • Add entry to the release notes

Cute Animal Picture

images

@prathmeshranaut prathmeshranaut force-pushed the prathmeshranaut/add-isort-linted branch 2 times, most recently from 6999ad2 to b57af94 Compare April 23, 2023 06:43
@prathmeshranaut prathmeshranaut changed the title Prathmeshranaut/add isort linted Linted file with isort in etc directory Apr 23, 2023
@prathmeshranaut prathmeshranaut force-pushed the prathmeshranaut/add-isort-linted branch 2 times, most recently from f9d16e6 to ef9b573 Compare April 23, 2023 06:56
@prathmeshranaut prathmeshranaut changed the title Linted file with isort in etc directory Linted file with isort in eth and tests directory Apr 23, 2023
@prathmeshranaut prathmeshranaut force-pushed the prathmeshranaut/add-isort-linted branch 2 times, most recently from dfcea1a to cbb591d Compare April 23, 2023 07:46
@prathmeshranaut prathmeshranaut changed the title Linted file with isort in eth and tests directory [2/3] Linted file with isort in eth and tests directory Apr 23, 2023
@prathmeshranaut prathmeshranaut force-pushed the prathmeshranaut/add-isort-linted branch 3 times, most recently from c5625e9 to e23faf1 Compare April 26, 2023 03:54
@prathmeshranaut
Copy link
Contributor Author

@pacrob Rebased with master and is ready for review.

@pacrob
Copy link
Collaborator

pacrob commented Apr 28, 2023

Hey @prathmeshranaut, thanks for moving on this. We're working on a refactor related to #2089, and it'll much easier get that merged then re-lint-roll than the other way around. I'll comment back here when we're ready to apply. Thanks!

@pacrob pacrob force-pushed the prathmeshranaut/add-isort-linted branch 2 times, most recently from ffc4e18 to 928cb1f Compare May 4, 2023 20:21
@pacrob pacrob force-pushed the prathmeshranaut/add-isort-linted branch from 928cb1f to 2f421ce Compare May 4, 2023 20:25
@pacrob
Copy link
Collaborator

pacrob commented May 4, 2023

Thanks for your help on this @prathmeshranaut ! I decided to just cherry-pick the "adding isort to tox" commit into this PR and made a couple small tweaks, and it should be ready to go!

Closes #2094
Closes #2103

@pacrob pacrob requested review from fselmo and kclowes May 4, 2023 20:58
@pacrob pacrob force-pushed the prathmeshranaut/add-isort-linted branch from 19286ad to cf14239 Compare May 4, 2023 21:52
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.

lgtm 👍🏼. Slight preference for putting the isort config into its own .isort.cfg file to keep tox.ini focused on tox-specific configs but we can figure that out at the template level later.

@pacrob pacrob merged commit fad4851 into ethereum:master May 4, 2023
@pacrob pacrob mentioned this pull request May 5, 2023
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