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

Use isort #5737

Closed
Gallaecio opened this issue Nov 27, 2022 · 3 comments · Fixed by #5806
Closed

Use isort #5737

Gallaecio opened this issue Nov 27, 2022 · 3 comments · Fixed by #5806

Comments

@Gallaecio
Copy link
Member

WARNING: This change should not be implemented before #5734 is merged, because there is a high chance of conflicts.

We should use isort to address import sorting and grouping:

  • Fix it for all existing code.
  • Automate it for new code.
  • Check it for new code, in case someone accidentally skips the automation.

Fixing existing code should be a matter of running isort . on the root folder and committing all changes.

Automation should be handled through pre-commit. Chances are will provide pre-commit support for black, so here it would be a matter of extending that to include isort:

https://github.com/scrapy-plugins/scrapy-zyte-api/blob/016ef98092d1795b317c44c11326eefa58215f0d/.pre-commit-config.yaml#L2-L5

It also seems like we may need an .isort.cfg file for compatibility with black:

https://github.com/scrapy-plugins/scrapy-zyte-api/blob/016ef98092d1795b317c44c11326eefa58215f0d/.isort.cfg

Finally, we need to check on a CI job that isort has been indeed used for all new changes. First we should add an entry to tox.ini:

[testenv:isort]
deps = 
    isort==5.10.1
commands = 
    isort --check-only {posargs:.}

Then, we can add a job to .github/workflows/checks.yml in line with the existing ones, i.e.:

        - python-version: "3.11"
          env:
            TOXENV: isort
@josh-nj-lawrence
Copy link

Hello @Gallaecio would it be alright if I give this issue a shot? I'm new to contributing to open source so apologize if I have questions but I'd be happy to help.

@Gallaecio
Copy link
Member Author

Gallaecio commented Dec 14, 2022

#5734 has not been merged yet, so it would not be a good idea to start with this already.

@josh-nj-lawrence
Copy link

#5734 has not been merged yet, so it would not be a good idea to start with this already.

No worries, I'll check out another issue to start with, perhaps 5734.

alexpdev added a commit to alexpdev/scrapy that referenced this issue Jan 15, 2023
@jxlil jxlil mentioned this issue Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants