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

Ruff enable isort (with custom sections) #2556

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

tdadela
Copy link
Contributor

@tdadela tdadela commented Jan 16, 2024

Enable ruff isort-like import sorting & make code compatible with them.

Alternative approach to #2554.

@cyberw
Copy link
Collaborator

cyberw commented Jan 17, 2024

Awesome! Can you just have a look at my comments and maybe test run one or two of the example locustfiles to see that nothing broke there?

Then this is ready for merge...

@tdadela
Copy link
Contributor Author

tdadela commented Jan 21, 2024

There are two independent problems to solve: circular imports and importing locust first in locustfiles.

  1. Circular imports in locust/locust folder.
    (The most important file is locust/locust/__init__.py).
    Python executes imports (and other expression) from top to down a file.
    Let's consider the following scenario:
    After sorting imports in locust/locust folder (without adding if TYPE_CHECKING: and moving some imports here) the from .user.users import HttpUser, User statement of the locust/locust/__init__.py is located in line 25, below for example from .shape import LoadTestShape (line 21)
    Let's consider following the one-line script:

    from locust import LoadTestShape

    from locust import LoadTestShape makes interpreter try to import LoadTestShape from locust/locust/__init__.py → interpreter opens locust/locust/__init__.py and reaches line 21 → opens shape.py file → reaches from . import User line BEFORE definition of LoadTestShape → from . import User means "import User from file locust/locust/__init__.py"
    However, it's not possible to import User because User is in line 25 and interpreter currently process line 21.
    I see three solutions to this problem:

    1. Making from .user.users import HttpUser, User the first import after monkey.patch_all(). It's possible thanks to the use of # isort: skip:
    from .user.users import HttpUser, User  # isort: skip
    1. Adding if TYPE_CHECKING in some files.
    2. Using from .user.users import User instead of from . import User in some files.
  2. In locustfiles Locust have to be imported first (due to gevent Monkey Patching).
    I see two solutions to this problem:

    1. Not sorting imports in locustfiles (like in Ruff enable isort #2554).
    2. Defining custom sorting section and custom selection order (see this PR – changes in pyproject.toml, documentation isort, documentation ruff).

@cyberw
Copy link
Collaborator

cyberw commented Jan 21, 2024

I see! I think option "1i" and "2ii" make the most sense - does that sound reasonable? Can you adjust your second PR? (or make a third one, if you prefer)

@cyberw
Copy link
Collaborator

cyberw commented Jan 22, 2024

Actually. Maybe the if TYPE_CHECKING makes more sense, as it shows the intent. I'll merge this as is, if you dont have anything more to add? (thanks for the explanations)

@tdadela
Copy link
Contributor Author

tdadela commented Jan 22, 2024

i, ii, iii (and any combination of them) looks good to me.

@cyberw cyberw merged commit 7b95b95 into locustio:master Jan 23, 2024
14 checks passed
@cyberw
Copy link
Collaborator

cyberw commented Jan 23, 2024

Huh. The build is failing after merge. Can you have a look? https://github.com/locustio/locust/actions/runs/7621650409/job/20758349498

@cyberw cyberw mentioned this pull request Jan 23, 2024
@cyberw
Copy link
Collaborator

cyberw commented Jan 23, 2024

nvm, I fixed it myself

@tdadela tdadela deleted the ruff_enable_isort_custo_sections branch January 23, 2024 20:24
@tdadela
Copy link
Contributor Author

tdadela commented Jan 23, 2024

(PR wasn't up-to-date with master branch. Some imports were added in the meantime.)

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