[fastapi] Implement fast-api-unused-path-parameter (FAST003)
#12638
Conversation
…t as a pos-only arg
Matthieu-LAURENT39
changed the title
|
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
|
Somehow VSCode didn't show the code blocks inside your reviews so i implemented that from scratch 😅 |
| if let Some((end, _)) = self.chars.by_ref().find(|&(_, ch)| ch == '}') { | ||
| let param_content = &self.input[start + 1..end]; | ||
| // We ignore text after a colon, since those are path convertors | ||
| // See also: https://fastapi.tiangolo.com/tutorial/path-params/?h=path#path-convertor |
There was a problem hiding this comment.
Since path parameters use the same format as f-strings, could we use our parser here instead? Just parse the path, extract the f-string segments? You can see here that its evaluated to a literal element, followed by an FStringExpressionElement where the : part is the FStringFormatSpec.
There was a problem hiding this comment.
Path parameters can use invalid identifier names. For example, /route/{path-name}, which the f-string parser doesn't support. However, we just ignore invalid identifiers in the rule anyways since fastapi normalizes them in an undocumented way, so i don't think this is a blocker to using that parser.
Also, fastapi doesn't handle the {name!r} and {name=} special syntaxes and would normalize them as name_r and name respectively, but this is a very obscure edge-case so it doesn't matter much. We do need to at least disable the rule if there is a conversion or debug_text in the f-string to avoid complaining about the wrong name though.
If those aren't a problem, i'll switch to the f-string parser
There was a problem hiding this comment.
I leave this up to you @charliermarsh but cloning the AST, then parsing it seems very heavy weight.
There was a problem hiding this comment.
Ok yeah, let's just skip the parser idea for now then.
Does FastAPI allow you to escape curly braces? Like /foo{{bar}}/ to represent a literal { and } in the path, as with f-strings?
There was a problem hiding this comment.
it doesn't, it will just be treated as if you had a path attribute named bar (without curly braces).
Although i'm pretty sure it's because of the aforementioned undocumented normalizing behavior.
Should i revert the commits using the f-string parser?
There was a problem hiding this comment.
Should i revert the commits using the f-string parser?
Yeah I think that would be great, considering that fast-api doesn't support f-strings?
CodSpeed Performance ReportMerging #12638 will not alter performanceComparing Summary
|
MichaReiser
added
rule
|
reverted to the old parsing system that doesn't use the f-string parser, as per the conversation |
|
Amazing! 🚀 |
charliermarsh
changed the title
fastapi] Implement fast-api-unused-path-parameter (FAST003)
This adds the
fast-api-unused-path-parameterlint rule, as described in #12632.I'm still pretty new to rust, so the code can probably be improved, feel free to tell me if there's any changes i should make.
Also, i needed to add the
add_parameteredit function, not sure if it was in the scope of the PR or if i should've made another one.