[flake8-comprehensions] Fix C420 to prepend whitespace when needed
#18616
Conversation
robsdedude
force-pushed
the
fix/18599-C420-prepend-padding
branch
from
9dae719 to
bdf0ffb
Compare
|
| /// identifier is ASCII-only or not by mutably altering a reference to a | ||
| /// boolean value passed in. | ||
| fn is_identifier_continuation(c: char, identifier_is_ascii_only: &mut bool) -> bool { | ||
| pub fn is_identifier_continuation(c: char, identifier_is_ascii_only: &mut bool) -> bool { |
There was a problem hiding this comment.
Not sure you're fine with this PR increasing the coupling of the crates. However, I didn't want to duplicate that logic. But maybe there's even a better general approach for fixing the issue that works on a higher logical level than chars.
There was a problem hiding this comment.
This is a neat find, I didn't know about this function before.
I think I would have instead reached for something more like was done in #17648 (checking if two token ranges are adjacent). Would that work here too?
I'm not totally against using the lexer function if not, but I'm not sure we really want to make it pub like you said.
There was a problem hiding this comment.
Could we use edits::pad_start instead or is_identifier. I'd prefer to not make this function public
There was a problem hiding this comment.
pad_start sounds exactly like what we want... However, looking at its implementation I suspect (not tested yet) that it's implemented incorrectly. It only considers is_ascii_alphabetic. Python's identifiers allow much more than ASCII alphabetic characters, which is exactly why I reached for is_identifier_continuation. How do you suggest to proceed? My gut feeling is
- use
pad_startfor this PR - open a separate issue to discuss fixing
pad_start
What is_identifier are you referring to? There are multiple functions with that name in the repo 😇 If you mean ruff_python_stdlib::identifiers::is_identifier, I suppose we could, but it'd require looking at more than just the preceding character which might only be a valid identifier continuation, but not a start (e.g., a digit). Sounds more costly and error prone.
There was a problem hiding this comment.
Well, maybe in this case it's correct because we only care about preceding keywords, which only are contain ASCII alphabetic chars.
There was a problem hiding this comment.
Yeah, after looking at some examples in the code-base as well as Python's grammar for expressions, I'm fairly convinced, that pad, pad_start, and pad_end are fine as long as all edits that are padded are replacing a full expression as those cannot be surrounded by arbitrary identifiers as far as I can tell.
ntBre
added
bug
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! This looks good overall, just a couple of suggestions.
| /// identifier is ASCII-only or not by mutably altering a reference to a | ||
| /// boolean value passed in. | ||
| fn is_identifier_continuation(c: char, identifier_is_ascii_only: &mut bool) -> bool { | ||
| pub fn is_identifier_continuation(c: char, identifier_is_ascii_only: &mut bool) -> bool { |
There was a problem hiding this comment.
This is a neat find, I didn't know about this function before.
I think I would have instead reached for something more like was done in #17648 (checking if two token ranges are adjacent). Would that work here too?
I'm not totally against using the lexer function if not, but I'm not sure we really want to make it pub like you said.
..._linter/src/rules/flake8_comprehensions/rules/unnecessary_dict_comprehension_for_iterable.rs
Outdated
Show resolved
Hide resolved
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! This looks really nice with pad_start. Thanks for double checking that it was working correctly too.
ntBre
changed the title
flake8_comprehensions] Fix C420 to prepend whitespace when neededflake8-comprehensions] Fix C420 to prepend whitespace when needed
astral-sh#18616) <!-- Thank you for contributing to Ruff/ty! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? (Please prefix with `[ty]` for ty pull requests.) - Does this pull request include references to any relevant issues? --> ## Summary This PR fixes rule C420's fix. The fix replaces `{...}` with `dict....(...)`. Therefore, if there is any identifier or such right before the fix, the fix will fuse that previous token with `dict...`. The example in the issue is ```python 0 or{x: None for x in "x"} # gets "fixed" to 0 ordict.fromkeys(iterable) ``` ## Related Issues Fixes: astral-sh#18599
3 participants
Summary
This PR fixes rule C420's fix. The fix replaces
{...}withdict....(...). Therefore, if there is any identifier or such right before the fix, the fix will fuse that previous token withdict....The example in the issue is
Related Issues
Fixes: #18599