Implement F522-F525 #899
Implement F522-F525 #899
Conversation
olliemath
force-pushed
the
main
branch
2 times, most recently
from
8e7934f
to
d8cb5b4
Compare
olliemath
force-pushed
the
main
branch
2 times, most recently
from
1572e5b
to
7db5eb4
Compare
|
I've pulled out the construction of I can't see a way to avoid cloning when building the failure messages, but that should hopefully be rare. |
|
This looks great! Thank you. (And yeah, we always clone when building checks and failure messages.) |
|
I assume this is good to go? Anything else from your perspective @olliemath? |
Since we work with refs, I don't see a way to avoid cloning when we pass along to a check (which can live longer than the function where we build the keywords), but when there are no issues, we don't perform any cloning.
Yup should be good to merge assuming the pipeline passes. |
|
By the way, I've been using interactive rebase to keep my commit history tidy - it looks like your general strategy is to squash+merge? In which case in future contributions I might abandon that approach and be a bit more liberal with my commits |
|
@olliemath - Yeah, happy to elaborate but I tend towards squashing such that the Git history corresponds to "one commit per PR". |
|
Awesome, thanks tons for this! |
|
Happy to help - esp as I'm super excited about using ruff myself |
Adds checks for F522 through F525. Closes #891
All of the new checks return
Noneif the format string is invalid (i.e. if F521 fails) - essentially I'm treating it like a syntax error and not processing further. I don't know if there's a neater way to do this, which works even if F521 is disabled.Most of the logic here is in the helper function
FormatSummary::try_from, which extracts all the keywords and indexes from the format string. Taking into account nested format strings like "{spam:{eggs}}" is what makes it a bit involved, so I've added some tests.The use of
HashSetin the summary keeps everything O(n) - for most typical python code, I'd speculate that aVecwould actually be quicker, however I can imagine somebody somewhere has a format function with 1000s of arguments for their own personal reasons and I wouldn't want that to come back to bite us. Use ofHashSetmeans that for F524 the missing placeholders may not be returned in the order they appear in the string literal - I don't know if that's enough of a big deal to warrant implementing something more complicated.