[FastAPI] Add sub-diagnostic explaining why a fix was unavailable (FAST002)#22565
[FastAPI] Add sub-diagnostic explaining why a fix was unavailable (FAST002)#22565ntBre merged 6 commits intoastral-sh:mainfrom
FastAPI] Add sub-diagnostic explaining why a fix was unavailable (FAST002)#22565Conversation
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
| 72 | pass | ||
| | | ||
| help: Replace with `typing_extensions.Annotated` | ||
| info: Fix unavailable: Cannot convert required parameter after optional parameter |
There was a problem hiding this comment.
I find the messaging here a bit confusing. It almost sounds as if there's no way to fix this violation (I'm also not fully convinced that we should add a sub-diagnostic like this, or that we should only show it when running ruff with -v).
Maybe something like: info: automatic fix is not available because
There was a problem hiding this comment.
yeah, I agree these can be worded better, let me update.
There was a problem hiding this comment.
Done, changed the messages to be a little bit more verbose & user friendly.
There was a problem hiding this comment.
I am also leaning towards making these subdiagnostics available in case of verbose mode(-v)
There was a problem hiding this comment.
Hmm, yeah maybe this wasn't a great suggestion for a sub-diagnostic, sorry! We could also consider just adding a Fix availability note to the documentation instead.
Alternatively, it might be more helpful to phrase the sub-diagnostic as a suggestion like "Reorder the arguments to enable an automatic fix" (but probably more specific than that).
Just a couple of ideas, I'm curious what y'all think.
There was a problem hiding this comment.
Given that users face the same issue as the fix -- they have to reorder the arguments or assign a default): Should we add a message making users aware that they have to be mindful of the default argument?
This makes the message useful beyond just saying that the auto-fix isn't available.
There was a problem hiding this comment.
done, worded the message a little more clear with suggestion to reorder the arguments.
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
|
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks, this is looking good! I just had one suggestion to simplify the code a bit and a couple of smaller ideas about the diagnostic message itself.
crates/ruff_linter/src/rules/fastapi/rules/fastapi_non_annotated_dependency.rs
Outdated
Show resolved
Hide resolved
...hots/ruff_linter__rules__fastapi__tests__fast-api-non-annotated-dependency_FAST002_0.py.snap
Outdated
Show resolved
Hide resolved
...hots/ruff_linter__rules__fastapi__tests__fast-api-non-annotated-dependency_FAST002_2.py.snap
Outdated
Show resolved
Hide resolved
Signed-off-by: Bhuminjay <bhuminjaysoni@gmail.com>
ntBre
changed the title
FastAPI] Add sub-diagnostic explaining why a fix was unavailable (FAST002)
3 participants
Summary
This PR fixes #22188 by adding autofix skipped reason as diagnostic info.
Test Plan
snapshots are updated accordingly.