Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Lint more cases in collapsible_if #14231

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Lint more cases in collapsible_if #14231

wants to merge 2 commits into from

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Feb 16, 2025 •
edited
Loading

Replace the use of Sugg::ast() which prevented combining if together when they contained comments by span manipulation.

A new configuration option lint_commented_code, which is true by default, allows opting out from this behavior.

If reviewed on GitHub, the second commit of this PR is best looked at side by side, with whitespace differences turned off.

changelog: [collapsible_if]: lint more cases

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 16, 2025
@samueltardieu samueltardieu mentioned this pull request Feb 16, 2025
Draft
@samueltardieu samueltardieu force-pushed the push-uopxwlslsuwp branch 2 times, most recently from b8d9f37 to a94f321 Compare February 16, 2025 15:32
@samueltardieu
Copy link
Contributor Author

I think I'll add a configuration option to merge if containing comments, as some people might not like it.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 18, 2025
@samueltardieu
Copy link
Contributor Author

Done, with the lint_commented_code option, true by default.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 18, 2025
@samueltardieu samueltardieu force-pushed the push-uopxwlslsuwp branch 2 times, most recently from 419a0a2 to 24c2681 Compare February 20, 2025 16:52
@samueltardieu samueltardieu force-pushed the push-uopxwlslsuwp branch 2 times, most recently from 6d030ab to f065214 Compare February 28, 2025 00:42
@samueltardieu samueltardieu changed the title Lint more cases in collapsible_if, without suggestions Lint more cases in collapsible_if Feb 28, 2025
@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Lint more cases in collapsible_if
ae82621 
Replace the use of `Sugg::ast()` which prevented combining `if`
together when they contained comments by span manipulation.

A new configuration option `lint_commented_code`, which is `true` by
default, opts out from this behavior.
@samueltardieu
Fix situations identified by collapsible_if new hits
affd2a7
@samueltardieu
Copy link
Contributor Author

Rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@llogiq llogiq

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@samueltardieu @rustbot @llogiq