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

Add UnnamedParameterUse rule #6055

Merged
merged 3 commits into from Oct 26, 2023
Merged

Add UnnamedParameterUse rule #6055

merged 3 commits into from Oct 26, 2023

Conversation

atulgpt
Copy link
Contributor

@atulgpt atulgpt commented Apr 29, 2023

Fixes #3534

@detekt-ci
Copy link
Collaborator

detekt-ci commented Apr 29, 2023 •
edited

Messages
📖 Thanks for adding a new rule to detekt ❤️
✅ We detekted that you added tests, to your rule, that's awesome!
✅

We detekted that you updated the default-detekt-config.yml file, that's awesome!

Generated by 🚫 dangerJS against 7bb86a5

@atulgpt atulgpt mentioned this pull request Apr 29, 2023
Closed
@codecov
Copy link

codecov bot commented Apr 29, 2023 •
edited

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (95a4218) 85.18% compared to head (7bb86a5) 85.17%.

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #6055      +/-   ##
============================================
- Coverage     85.18%   85.17%   -0.01%     
- Complexity     4037     4061      +24     
============================================
  Files           564      565       +1     
  Lines         13273    13318      +45     
  Branches       2387     2401      +14     
============================================
+ Hits          11306    11343      +37     
- Misses          773      774       +1     
- Partials       1194     1201       +7
Files Coverage Δ
...turbosch/detekt/rules/bugs/PotentialBugProvider.kt 100.00% <100.00%> (ø)
...rturbosch/detekt/rules/bugs/UnnamedParameterUse.kt 81.81% <81.81%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

BraisGabin
BraisGabin reviewed May 6, 2023
View reviewed changes
Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

This issue doesn't treat booleans as any special type, right? I was reading again my initial proposal on the issue and I'm not sure if my initial issue is solved.

Extracted from there:

Noncompliant

User("Brais", false)

getUser(id, true)

Compliant

User("Brais", isActive = false)

getUser(id, forceRefresh = true)

getUser(id, refresh)

...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
...errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUseSpec.kt Show resolved Hide resolved
@BraisGabin
Copy link
Member

Is it feasible? Should we have a configuration to say which types should always use name arguments? Should we only consider booleans if they are the literals?

@atulgpt
Copy link
Contributor Author

atulgpt commented May 6, 2023 •
edited

This issue doesn't treat booleans as any special type, right? I was reading again my initial proposal on the issue and I'm not sure if my initial issue is solved.

Extracted from there:

Noncompliant

User("Brais", false)

getUser(id, true)

Compliant

User("Brais", isActive = false)

getUser(id, forceRefresh = true)

getUser(id, refresh)

No @BraisGabin this rule is general to any type and doesn't treat boolean as a special type. I followed from comment at #3534 (comment)

@atulgpt
Copy link
Contributor Author

atulgpt commented May 6, 2023 •
edited

Is it feasible? Should we have a configuration to say which types should always use name arguments? Should we only consider booleans if they are the literals?

So basically onlyCheckLiteral will have no effect on non-primitive data types. Right? Also, are onlyCheckLiteral and typesToCheck correct config names?

@atulgpt
Copy link
Contributor Author

atulgpt commented May 15, 2023

Is it feasible? Should we have a configuration to say which types should always use name arguments? Should we only consider booleans if they are the literals?

So basically onlyCheckLiteral will have no effect on non-primitive data types. Right? Also, are onlyCheckLiteral and typesToCheck correct config names?

^ @BraisGabin

@detekt-ci
Copy link
Collaborator

This PR is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

@BraisGabin BraisGabin removed the stale label Aug 14, 2023
@BraisGabin BraisGabin self-requested a review August 14, 2023 07:07
@cortinico cortinico added this to the 2.0.0 milestone Aug 19, 2023
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Aug 19, 2023
@schalkms
Copy link
Member

I'm not sure whether this rule should be included into the default rule set. I have the feeling that this rule ends up way more controversial than we expect. Let's see what others think about.

Anyhow, the rule is a good fit for the detekt market place. 🙂

@atulgpt
Copy link
Contributor Author

atulgpt commented Aug 21, 2023

I'm not sure whether this rule should be included into the default rule set. I have the feeling that this rule ends up way more controversial than we expect. Let's see what others think about.

Anyhow, the rule is a good fit for the detekt market place. 🙂

Hi, @schalkms I think this can go to detekt given we have NamedArguments which deals with similar areas but in a more general way. While this rule helps in actually detecting the cases when bugs can occur when names are not provided. There is more discussion in the GitHub comment at #3534 (comment) which mentions youtrack issue as well.
But agree let's hear other opinions as well

VovaStelmashchuk
VovaStelmashchuk reviewed Sep 25, 2023
View reviewed changes
...les-errorprone/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/UnnamedParameterUse.kt Outdated

val namedArgumentList = valueArgumentList.arguments.map {
// No name parameter if it is vararg
(it.isNamed() || paramDescriptorToArgumentMap[it]?.isVararg == true) to it
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move (it.isNamed() || paramDescriptorToArgumentMap[it]?.isVararg == true) to some new field, like a
val isParametrNamed = . I think it will make code more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use the isNamedOrVararg variable

atulgpt and others added 2 commits September 26, 2023 00:05
@atulgpt
Add UnnamedParameterUse rule
071d96a
@atulgpt @BraisGabin
Apply suggestions from code review
5d0e53a 
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
@atulgpt
Add review comments
7bb86a5 
Rebase from the upstream main
@cortinico cortinico merged commit 100eb5d into detekt:main Oct 26, 2023
23 checks passed
@atulgpt atulgpt deleted the feat/3534/add-named-arg-rule branch October 27, 2023 09:03
@TWiStErRob TWiStErRob mentioned this pull request Dec 3, 2023
Merged
mgroth0 pushed a commit to mgroth0/detekt that referenced this pull request Feb 11, 2024
@atulgpt @BraisGabin @mgroth0
Add UnnamedParameterUse rule (detekt#6055)
7013f59 
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
@DHosseiny
Copy link

Is this rule going to add to the detekt rules?

@BraisGabin
Copy link
Member

It will, at detekt 2.0

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

@VovaStelmashchuk VovaStelmashchuk VovaStelmashchuk left review comments

@BraisGabin BraisGabin BraisGabin approved these changes

@cortinico cortinico cortinico approved these changes

Assignees
No one assigned
Labels
notable changes Marker for notable changes in the changelog rules
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

Always use Named Arguments with booleans
7 participants
@atulgpt @detekt-ci @BraisGabin @schalkms @DHosseiny @cortinico @VovaStelmashchuk