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

[Bug]: lint_ruff_aspect does not fully support sorting imports #303

Open
honnix opened this issue Jun 28, 2024 · 4 comments
Open

[Bug]: lint_ruff_aspect does not fully support sorting imports #303

honnix opened this issue Jun 28, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@honnix
Copy link
Contributor

honnix commented Jun 28, 2024

What happened?

When enabling sorting imports in e.g. pyproject.toml, there are cases ruff cannot categorize an import to be StandardLibrary, FirstParty, or ThirdParty, because it does not see a full source tree.

Version

Development (host) and target OS/architectures:

Output of bazel --version: bazel 7.2.1

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file: v1.0.0-rc4

Language(s) and/or frameworks involved: Python

How to reproduce

Consider a simple source tree as the following:

src
|_foo.py
|_foo_test.py

where foo_test.py has imports like:

import known_third_party
import foo

pyproject.toml:

[tool.ruff]
src = ["src"]

[tool.ruff.lint]
extend-select = ["I"]

And the following bazel stuff:

py_binary(
    name = "foo",
    srcs = ["foo.py"],
)

py_test(
    name = "foo_test",
    srcs = ["foo_test.py"],
    deps = [":foo"]
)

ruff = lint_ruff_aspect(
    binary = "@@//path/to:ruff",
    configs = ["@@//:pyproject.toml"],
)

ruff_test = lint_test(aspect = ruff)

ruff_test(
    name = "ruff_test",
    srcs = [":foo_test"],
)

bazel test //path/to:ruff_test would fail because ruff cannot find foo.py so it categorized it as a ThirdParty, which means it wants this instead:

import foo
import known_third_party

Any other information?

After a closer look at this, it seems lint_ruff_aspect only populates srcs to be in the sandbox source tree, which means only src/foo_test.py is in the source tree, not deps.

It also doesn't help having:

ruff_test(
    name = "ruff_test",
    srcs = [":foo", ":foo_test"],
)

because it would just be two separated ruff invocations, one with foo.py in source tree and the other one with foo_test.py in source tree.

Another thing to point out is, lint_ruff_aspect does not support having pyproject.toml not in project root. I left a comment here.

@honnix honnix added the bug Something isn't working label Jun 28, 2024
@alexeagle
Copy link
Member

It's easy enough to pass ruff the transitive sources as inputs. I'm curious if you know of other cases where they are needed? The downside is that a file edit will now invalidate lint actions for all reachable nodes in the graph, making it slower.

@honnix
Copy link
Contributor Author

honnix commented Jul 1, 2024

I'm not sure of other cases.

The downside is that a file edit will now invalidate lint actions for all reachable nodes in the graph, making it slower.

I don't fully understand this. Could you please elaborate a bit more? Thanks.

@alexeagle
Copy link
Member

alexeagle commented Jul 5, 2024

If app/main.py imports from lib/helper.py, then edits to helper.py will now invalidate the linting for the py_library or py_binary in the app/ folder now, where previously it wouldn't.
That's potentially okay - it's required for type-checkers to behave this way. But, if it's only to support a few sorted imports appearing differently, then it's too bad to burn engineers time and compute resources on a lot of re-linting when a base library changes.

@honnix
Copy link
Contributor Author

honnix commented Jul 8, 2024

Thank you for explaining. And yeah I agree with the assessment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants