Add support for configuring knot in `pyproject.toml` files by MichaReiser · Pull Request #15493 · astral-sh/ruff

MichaReiser · 2025-01-15T10:55:55Z

Summary

This PR adds support for configuring Red Knot in the tool.knot section of the project's
pyproject.toml section. Options specified on the CLI precede the
options in the configuration file.

This PR only supports the environment and the src.root options for now.
Other options will be added as separate PRs.

There are also a few concerns that I intentionally ignored as part of this PR:

Handling of relative paths: We need to anchor paths relative to the current working directory (CLI), or the project (pyproject.toml or knot.toml)
Tracking the source of a value. Diagnostics would benefit from knowing from which configuration a value comes so that we can point the user to the right configuration file (or CLI) if the configuration is invalid.
Schema generation and there's a lot more; see Configuration ty#219

This PR changes the default for first party codes: Our existing default was to only add the project root. Now, Red Knot adds the project root and src (if such a directory exists).

Theoretically, we'd have to add a file watcher event that changes the first-party search paths if a user later creates a src directory. I think this is pretty uncommon, which is why I ignored the complexity for now but I can be persuaded to handle it if it's considered important.

Part of astral-sh/ty#219

Test Plan

Existing tests, new file watching test demonstrating that changing the python version and platform is correctly reflected.

github-actions · 2025-01-15T11:04:37Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

warning: Detected debug build without --no-cache.
error: Failed to read examples/Assistants_API_overview_python.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: expected `,` or `]` at line 197 column 8

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

openai/openai-cookbook (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to read examples/Assistants_API_overview_python.ipynb: Expected a Jupyter Notebook, which must be internally stored as JSON, but this file isn't valid JSON: expected `,` or `]` at line 197 column 8

crates/red_knot/src/main.rs

crates/red_knot_python_semantic/src/module_resolver/testing.rs

MichaReiser · 2025-01-15T11:11:07Z

crates/red_knot_workspace/src/project/options.rs

+/// The options for the project.
+#[derive(Debug, Default, Clone, PartialEq, Eq, Combine, Serialize, Deserialize)]
+#[serde(rename_all = "kebab-case", deny_unknown_fields)]
+pub struct Options {


I renamed Configuration to Options to match uv's and ruff's terminology.

crates/red_knot/src/main.rs

crates/red_knot/tests/cli.rs

sharkdp · 2025-01-15T16:09:11Z

crates/red_knot_workspace/src/project/combine.rs

+/// a CLI interface where values provided later can override values provided earlier: `knot --ignore=correctness --warn=correctness/rule`
+/// should enable `correctness/rule` even though `correctness` was previously ignored.


This particular example is a bit confusing to me, as --ignore and --warn seem like two (related but) different options?

The inclusion/exclusion patterns might be a better example a merging of something like exclude = ["*.png"] and exclude = ["!important.png"] to ["*.png", "!important.png"]?

I updated the comment and also pointed out a potential "shortcoming" of this approach. I'm interested to hear if there are any concerns or if we feel comfortable moving forward with how it is

I can certainly imagine cases where we would want a "replace" logic for lists, instead of a "extend" logic. But I think it's completely fine to try and see how far this approach takes us. Thank you for the updated description!

crates/red_knot/tests/file_watching.rs

crates/red_knot_workspace/src/project/combine.rs

crates/ruff_macros/src/combine.rs

crates/red_knot_workspace/src/project/combine.rs

carljm · 2025-01-15T18:54:04Z

crates/red_knot/src/main.rs

+    let mut workspace_metadata = ProjectMetadata::discover(system.current_directory(), &system)?;
+    workspace_metadata.apply_cli_options(cli_options.clone());

    // TODO: Use the `program_settings` to compute the key for the database's persistent


program_settings looks maybe like an out-of-date variable name here?

I moved it into ProjectDatabase::new where this now fits better

crates/red_knot_python_semantic/src/program.rs

crates/red_knot_python_semantic/src/module_resolver/testing.rs

MichaReiser · 2025-01-15T21:40:57Z

Reading through the comments. It looks like this is good to land for as long as I undo the rename from venv-path to python

This PR adds support for configuring Red Knot in the `tool.knot` section of the project's `pyproject.toml` section. Options specified on the CLI take precedence over the options in the configuration file. For now, this PR only adds support for the `environment` and the `src.root` options. Other options will be added as separate PRs. There are also a few concerns that I intentionally ignored as part of this PR: * Handling of relative paths: We need to anchor paths relative to the current working directory (CLI), or the project (`pyproject.toml` or `knot.toml`) * Tracking the source of a value. It would be nice for diagnostics to know from which configuration a value comes so that we can point the user to the right configuration file (or CLI) if the configuration is invalid. * Schema generation, and there's a lot more, see https://github.com/astral-sh/ruff/issues/15491

crates/red_knot_workspace/src/project/options.rs

sharkdp

Thank you, looks great!

I'm not yet familiar with the file_watching part, so I only skimmed that. Let me know if I should do a more detailed review.

* main: [red-knot] Inline `SubclassOfType::as_instance_type_of_metaclass()` (#15556) [`flake8-comprehensions`] strip parentheses around generators in `unnecessary-generator-set` (`C401`) (#15553) [`pylint`] Implement `redefined-slots-in-subclass` (`W0244`) (#9640) [`flake8-bugbear`] Do not raise error if keyword argument is present and target-python version is less or equals than 3.9 (`B903`) (#15549) [red-knot] `type[T]` is disjoint from `type[S]` if the metaclass of `T` is disjoint from the metaclass of `S` (#15547) [red-knot] Pure instance variables declared in class body (#15515) Update snapshots of #15507 with new annotated snipetts rendering (#15546) [`pylint`] Do not report methods with only one `EM101`-compatible `raise` (`PLR6301`) (#15507) Fix unstable f-string formatting for expressions containing a trailing comma (#15545) Support `knot.toml` files in project discovery (#15505) Add support for configuring knot in `pyproject.toml` files (#15493) Fix bracket spacing for single-element tuples in f-string expressions (#15537) [`flake8-simplify`] Do not emit diagnostics for expressions inside string type annotations (`SIM222`, `SIM223`) (#15405) [`flake8-pytest-style`] Do not emit diagnostics for empty `for` loops (`PT012`, `PT031`) (#15542)

MichaReiser added the ty Multi-file analysis & type inference label Jan 15, 2025

MichaReiser force-pushed the micha/knot-configuration branch from 17e209e to 6487e53 Compare January 15, 2025 11:15

MichaReiser commented Jan 15, 2025

View reviewed changes

MichaReiser force-pushed the micha/knot-configuration branch 2 times, most recently from 93875ec to f207805 Compare January 15, 2025 13:04

MichaReiser marked this pull request as ready for review January 15, 2025 13:04

MichaReiser requested review from AlexWaygood, carljm and sharkdp as code owners January 15, 2025 13:04