Skip to content

Conversation

@charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Jun 8, 2025

Closes #17683.

@charliermarsh charliermarsh added bug Something isn't working fixes Related to suggested fixes for violations labels Jun 8, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@charliermarsh charliermarsh merged commit 301b9f4 into main Jun 8, 2025
34 checks passed
@charliermarsh charliermarsh deleted the charlie/read branch June 8, 2025 16:00
Comment on lines +107 to +109
print([line for line in f.readlines()and True])
print([line for line in f.readlines()or True])
print([line for line in f.readlines()in ["test"]])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review, but these three cases don't actually trigger FURB129, so I'm not sure if they're necessary. I noticed this while rebasing the 0.12 branch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is sort of marginal -- I see some value / little cost in having these kinds of pathological cases here just to make sure they don't cause a panic, but taking that attitude to the extreme is obviously untenable :) I can remove since I'll be doing a follow-up PR anyway, but if I weren't changing the other piece in readlines_in_for.rs, I'd probably opt to just leave these here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I can see the value too! I was just afraid I dropped a few diagnostics while rebasing until I realized they weren't meant to trigger. I'm happy either way in both cases here.

Comment on lines +107 to +109
} else {
Edit::range_replacement(padded, deletion_range)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm here, I also think this section would be a bit nicer as:

    let deletion_range = if let Some(parenthesized_range) = parenthesized_range(
        expr_attr.value.as_ref().into(),
        expr_attr.into(),
        checker.comment_ranges(),
        checker.source(),
    ) {
        expr_call.range().add_start(parenthesized_range.len())
    } else {
        expr_call.range().add_start(expr_attr.value.range().len())
    };

    let padded = pad_end(String::new(), deletion_range.end(), checker.locator());
    let edit = if padded.is_empty() {
        Edit::range_deletion(deletion_range)
    } else {
        Edit::range_replacement(padded, deletion_range)
    };

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change!

charliermarsh added a commit that referenced this pull request Jun 9, 2025
## Summary

Post-merge feedback from #18542.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working fixes Related to suggested fixes for violations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FURB129 fix should add spaces between tokens

3 participants