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.

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

Collapse if and if let chains in Clippy itself #14228

Closed

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Feb 16, 2025

This adds a collapse_let_chains config option to the collapsible_if lint. This lint is then applies to Clippy itself. This reduces the level of indentation in Clippy source code in many locations.

The first two commits are from PR #14231, the third one from PR #14233. They must be merged first (or as part of this PR).

If reviewed on GitHub, the last three commits of this PR are best looked at side by side, with whitespace differences turned off.

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2025

r? @Centri3

rustbot has assigned @Centri3.
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 marked this pull request as draft February 16, 2025 12:44
@samueltardieu samueltardieu force-pushed the push-ptpxkoyowrpq branch 3 times, most recently from 4eab1e1 to b68a741 Compare February 16, 2025 14:26
@samueltardieu samueltardieu marked this pull request as ready for review February 16, 2025 14:27
@samueltardieu samueltardieu force-pushed the push-ptpxkoyowrpq branch 3 times, most recently from 99ccf9c to b78eba0 Compare February 16, 2025 15:32
@samueltardieu
Copy link
Contributor Author

samueltardieu commented Feb 16, 2025

Way too many hits from lintcheck, will double check.

Edit: lintcheck from the CI seems to use clippy.toml from the project, so it sets the collapse-let-chains option. I'll have to find a way to disable this during the lintcheck lint.
Edit: solved through #14233

@samueltardieu samueltardieu marked this pull request as draft February 16, 2025 15:46
@samueltardieu samueltardieu force-pushed the push-ptpxkoyowrpq branch 2 times, most recently from ea66a22 to fd78e73 Compare February 16, 2025 16:39
@samueltardieu samueltardieu marked this pull request as ready for review February 16, 2025 16:50
@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2025

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits:

@samueltardieu
Copy link
Contributor Author

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

Those will disappear after the PR it depends on are merged – I'll push with a linear history then, which will contain the same content.

@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2025

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2025

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

@rustbot
Copy link
Collaborator

rustbot commented Feb 18, 2025

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2025

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust-clippy.git master
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

@samueltardieu
Copy link
Contributor Author

I'll put this on hold until let chains are stabilized.
r? ghost
@rustbot author

@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2025

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@samueltardieu samueltardieu marked this pull request as draft February 23, 2025 22:02
@samueltardieu samueltardieu removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 23, 2025
@samueltardieu samueltardieu force-pushed the push-ptpxkoyowrpq branch 4 times, most recently from 4b2e944 to 36e0eed Compare February 28, 2025 23:01
@samueltardieu
Copy link
Contributor Author

Rebased

@samueltardieu
Copy link
Contributor Author

Rebased

@Centri3
Copy link
Member

Centri3 commented Mar 20, 2025

Just based off a quick look I'm completely ok with merging this before stabilization, we've already had lints like allow_attributes in the past

@samueltardieu samueltardieu force-pushed the push-ptpxkoyowrpq branch 2 times, most recently from 77cf95b to b5a2e15 Compare March 23, 2025 17:12
@samueltardieu
Copy link
Contributor Author

samueltardieu commented Mar 23, 2025

Just based off a quick look I'm completely ok with merging this before stabilization, we've already had lints like allow_attributes in the past

This needs to be done in steps, as it uses non-default values for options when linting Clippy itself. This requires turning the option back to its default value for lintcheck, which in turn requires the option to be known for lintcheck base to work as expected.

@flip1995 already merged #14231, now #14451 is ready, then I'll assign #14455 to you (first part of this PR), then this one will be ready for review (containing only the latest commit).

@samueltardieu samueltardieu removed the has-merge-commits PR has merge commits, merge with caution. label Mar 24, 2025
@samueltardieu samueltardieu force-pushed the push-ptpxkoyowrpq branch 3 times, most recently from 52be538 to 2a5f065 Compare March 25, 2025 18:50
github-merge-queue bot pushed a commit that referenced this pull request Mar 25, 2025
This PR enables the new ability to collapse `if` statements containing
comments (without losing them) in Clippy sources, excluding tests and
lintcheck, where the default behaviour (no collapsing in presence of
comments) is preserved.

To be applied after #14231. When it is applied, #14455 will be marked as
ready for review, then #14228 afterwards.

changelog: none

r? ghost
Since `collapsible_if` is an early lint, it is not possible to
automatically check whether the `let_chains` unstable rustc feature is
enabled or not, as features are not available in the `EarlyContext`
object.

For this reason, the `collapse-let-chains` needs to be enabled
explicitly. This can be reexamined later when the `let_chains` feature
is stabilized.
tests and lintcheck runs are not affected, and will use the default
non-collapsing setting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants