-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[red-knot] User-level configuration #16021
Conversation
@@ -99,10 +99,12 @@ fn run_check(args: CheckCommand) -> anyhow::Result<ExitStatus> { | |||
let exit_zero = args.exit_zero; | |||
|
|||
let cli_options = args.into_options(); | |||
let mut workspace_metadata = ProjectMetadata::discover(system.current_directory(), &system)?; | |||
workspace_metadata.apply_cli_options(cli_options.clone()); | |||
let mut project_metadata = ProjectMetadata::discover(system.current_directory(), &system)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want nicer error messages here that use proper diagnostics but I decided to not tackle that for now and continue doing what we do for other errors at this stage.
@@ -47,7 +48,7 @@ impl ProjectDatabase { | |||
if let Some(path) = change.system_path() { | |||
if matches!( | |||
path.file_name(), | |||
Some(".gitignore" | ".ignore" | "ruff.toml" | ".ruff.toml" | "pyproject.toml") | |||
Some(".gitignore" | ".ignore" | "knot.toml" | "pyproject.toml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops
if let Err(error) = metadata.apply_configuration_files(self.system()) { | ||
tracing::error!( | ||
"Failed to apply configuration files, continuing without applying them: {error}" | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to use proper diagnostics here so that LSP users get notified about this error but there are other errors in this file where we're using tracing
-> A problem for another day
if entry | ||
.path() | ||
.extension() | ||
.and_then(PySourceType::try_from_extension) | ||
.is_some() | ||
{ | ||
let mut paths = added_paths.lock().unwrap(); | ||
|
||
paths.push(entry.into_path()); | ||
paths.push(entry.into_path()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got really confused when red knot started parsing my knot.toml
file as a python file. For now, I did the simplest fix by copying this condition from Project::discover_files
. I have to rework this code next month anyway to properly support file inclusion and exclusions.
39bef15
to
a1ab60d
Compare
crates/red_knot/tests/utils.rs
Outdated
#![allow(dead_code)] | ||
|
||
/// Sets an environment variable for the duration this struct is in scope. | ||
pub(crate) struct EnvVarGuard { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually not sufficient because cargo runs tests concurrently. So we also need some form of a lock to avoid that multiple tests hold on to the same env variable. Ugh, this is getting out of hand.
b2aeb58
to
dd75dc2
Compare
|
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
6b78710
to
902cdb4
Compare
4355d5d
to
521cc67
Compare
118ae5b
to
1dbf7de
Compare
* main: add diagnostic `Span` (couples `File` and `TextRange`) (#16101) Remove `Hash` and `Eq` from `AstNodeRef` for types not implementing `Eq` or `Hash` (#16100) Fix release build warning about unused todo type message (#16102) [`pydocstyle`] Handle arguments with the same names as sections (`D417`) (#16011) [red-knot] Reduce usage of `From<Type>` implementations when working with `Symbol`s (#16076) Transition to salsa coarse-grained tracked structs (#15763) [`pyupgrade`] Handle micro version numbers correctly (`UP036`) (#16091) [red-knot] `T | object == object` (#16088) [`ruff`] Skip singleton starred expressions for `incorrectly-parenthesized-tuple-in-subscript` (`RUF031`) (#16083) Delete left-over `verbosity.rs (#16081) [red-knot] User-level configuration (#16021) Add `user_configuration_directory` to `System` (#16020)
Summary
This PR adds support for user-level configurations (
~/.config/knot/knot.toml
) to Red Knot.Red Knot will watch the user-level configuration file for changes but only if it exists
when the process start. It doesn't watch for new configurations,
mainly to simplify things for now (it would require watching the entire
.config
directory because theknot
subfolder might not exist either).The new
ConfigurationFile
struct seems a bit overkill for now but I plan to use it forhierarchical configurations as well.
Red Knot uses the same strategy as uv and Ruff by using the etcetera crate.
Test Plan
Added CLI and file watching test