-
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
better error message when an inherited config path doesn't exist #15658
base: main
Are you sure you want to change the base?
better error message when an inherited config path doesn't exist #15658
Conversation
|
b8c2fbe
to
730bae4
Compare
let options = pyproject::load_options(&path)?; | ||
let options = pyproject::load_options(&path).map_err(|err| match inherited_by { | ||
Some(f) => err.context(format!( | ||
"Failed to load path {} inherited by {}", |
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 find the wording here slightly confusing. It suggests that the path is inherited where it is the configuration.
I also think we should track the entire stack
because the message is now inprecise in case there's a inheritance chain like a -> b -> c
because the message only mentions that b
extends c
without mentioning a
.
There are also other paths where this method returns an error (e.g. bail) where the inheritance chain will be missing.
That's why I suggest making the following changes:
- Store the path in the
stack
so that we get the entire inheritance chain instead of just the last - move the body of the while loop into a function (or lambda) to ensure we attach the context to all errors returned in the while loop
- Change the error to
Failed to load configuration
{path}: error ({chain})
wherechain
is only shown if the stack isn't empty (and it showsa -> b -> c)
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.
Hmm in the case of a -> b -> c
, if c
doesn't exist, why do we care about a
? Fixing the extends
clause in b
should be enough, and providing the full chain might be confusing vs just providing the file that caused the error, and which file tried to extend it. The meaning of the direction of the arrow could be confusing too.
However, down to make the changes and my opinion isn't too strong here. I'll take a stab tomorrow.
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.
That makes sense. I understood that the main motivation is that it's hard to understand why a file gets imported (how does Ruff end up with that given path and where can I change it). That's why I thought it's helpful if we show the entire inheritance chain over letting users guess how Ruff ended uploading that file, but it might go beyond of what the goal of this PR is.
I'm open to use anything other than -> if you consider its meaning ambigious. We could also use
aextends
b`
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.
Nice, this is a great improvement. I've some suggestions on how we can improve the message further. It would probably also be good to add a CLI test for it, similar to what we have here
Currently, if a ruff config extends a non-existent file via
extend =
, youget an incomplete error message that could be very misleading (I spent quite some
time wondering why ruff was trying to load a pyproject I deleted, and found out
what was going on after building a local ruff with tons of prints).
Previously:
Now: