Enable isort in the pre-commit tool settings

[webknjaz]

No description provided.

[webknjaz]

@abhinavsingh this one corresponds best to the style you already use in the repo but I usually prefer 5: https://pycqa.github.io/isort/docs/configuration/multi_line_output_modes.html

[abhinavsingh]

SGTM. We can use 5 which is reasonable limit.

I am unsure what will be the implications. I guess need to split multiple grouped imports or may be longer import lines? Irrespective, happy to get this in. Let's keep it consistent with aio-libs which is already accepted widely in the developer community :)

[webknjaz]

It's not a limit, it's a style (or "mode") of formatting, follow the lint and you'll see. aiohttp actually uses 3 for some reason but I didn't set that up and try not to change the formatting settings in that project.

[abhinavsingh]

"isort your imports, so you don't have to."

I see what you mean now. It's part of a fixture config :).

Happy to see this integration, indeed following a standard is the way to go, I was sorting them based upon a random in-my-brain-only logic 🗡️

[webknjaz]

Yeah, also one of the most common import orders is to have stdlib at the top, followed by third-party imports and finally project-local. But with isort you can define extra sections if some things have additional semantical meaning within the project or explicitly declare that some things belong to certain sections.

[webknjaz]

Wow, I see that the failing tests reveal some import loops present in the project. Not sure how hard would it be to solve them.

[abhinavsingh]

Wow, I see that the failing tests reveal some import loops present in the project. Not sure how hard would it be to solve them.

Code has gone through a lot of changes between versions (from a single file to module based), so I am not surprised this can pop up after import order change :).

Another reason for circular import errors could be type checking requirements. I do remember importing classes from other modules, just to define a typed variable. This can cause issue. From top-of-the-head, flags could be another culprit here, as it is imported by literally every file in the repo. But will need to dig deep to find the root cause(s).

Ideally most of the sub-modules are written in isolations. But they may have reminiscence from v1 code.

[webknjaz]

I'll probably try adding a pytest test that I picked up from the pytest project itself, for catching circular imports.

[webknjaz]

I'll probably try adding a pytest test that I picked up from the pytest project itself, for catching circular imports.

Here you go: #678

[abhinavsingh]

@webknjaz I added isort support recently in another private project of mine. Wondering if we could still enable it for proxy.py. For now I am closing this, as most likely we might need to re-sync the branch and resolve conflicts. A lot has changed since this PR. Let's re-open or create a new one once ready. I'll also spend sometime and see how can we get isort in.

[webknjaz]

In order to enable isort, you'd need to solve the import loops it causes but I imagine you could try enabling it for a part of the project and not all of the files and then gradually include more modules as you sort out the problems.

[abhinavsingh]

Sounds good, let me take a look. Currently unsure what are these circular import issues. The circular import test we added seems to catch no issues. Will report back my findings.