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

Reduce configuration of UnusedPrivateMember's split rules #5800

Merged
merged 4 commits into from Feb 24, 2023

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented Feb 21, 2023

Those allowedNames were copy&pasted when UnusedPrivateMember was splitted. But some of them have no sense in some usages.

This is a follow up of #5722

Comment on lines 706 to +716
UnusedParameter:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
allowedNames: 'ignored|expected'
UnusedPrivateClass:
active: true
UnusedPrivateMember:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
allowedNames: ''
UnusedPrivateProperty:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
allowedNames: '_|ignored|expected|serialVersionUID'
Copy link
Member

Choose a reason for hiding this comment

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

feels like _ should be a parameter name, i.e.

foo.bar { _, x -> ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

It does... we could open an issue and move that to the other rule.

also I created this test case and we have a lot of false negatives regarding lambdas:

Pair(1, 2).let { (x, y) -> println(y) }
listOf<Int>().map { x -> 1 }.fold(a) { x, y -> y }
val foo: (String, String) -> Unit = { x, y -> println(y) }

And I think that we don't even should configure _ as an unused parameter. That's exactly the reason _ exists.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but otherwise it would be a false positive, isn't it?

I didn't know there were so many gaps, my point was that you added _ as private property, but not for parameter names, now I tried and it's not allowed:
image
image
image

so it's pretty much only allowed as a lambda parameter.

Copy link
Member

Choose a reason for hiding this comment

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

@BraisGabin based on ^, it looks like we should just remove the _ exception.

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 didn't remove it from the property rule because, otherwise, we have false positives right now. We should probably take a look at it. I don't think the lambda check should be in the property rule and I think that we should omit those by default. But that's an improvement out of scope for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, the lambda param is a "private property", sorry, I missed that.

Copy link
Member

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

I was wondering why these were set up the the way they are. Nice one 👏

@TWiStErRob TWiStErRob changed the title Reduce configuration Reduce configuration of UnusedPrivateMember's split rules Feb 22, 2023
@BraisGabin BraisGabin merged commit 2367650 into main Feb 24, 2023
@BraisGabin BraisGabin deleted the reduce-configuration branch February 24, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants