Skip to content

Define FunctionSignature #4176

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

Merged
merged 4 commits into from
Nov 6, 2021
Merged

Define FunctionSignature #4176

merged 4 commits into from
Nov 6, 2021

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented Oct 9, 2021

This class is a helper that I need to implement #4148. I moved it outside that PR becase the code is complex and have entity by itself.

I'm not very happy with all the naming that I'm using so any suggestion there is that regard is more than welcome.

@codecov
Copy link

codecov bot commented Oct 9, 2021

Codecov Report

Merging #4176 (d7974ef) into main (9aa0407) will decrease coverage by 0.00%.
The diff coverage is 87.87%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4176      +/-   ##
============================================
- Coverage     84.26%   84.25%   -0.01%     
+ Complexity     3245     3238       -7     
============================================
  Files           468      469       +1     
  Lines         10178    10199      +21     
  Branches       1787     1792       +5     
============================================
+ Hits           8576     8593      +17     
  Misses          658      658              
- Partials        944      948       +4     
Impacted Files Coverage Δ
.../gitlab/arturbosch/detekt/rules/MethodSignature.kt 70.00% <ø> (ø)
...in/io/github/detekt/tooling/api/FunctionMatcher.kt 86.20% <86.20%> (ø)
...turbosch/detekt/rules/style/ForbiddenMethodCall.kt 88.57% <100.00%> (+0.76%) ⬆️
...rbosch/detekt/rules/style/ObjectLiteralToLambda.kt 83.78% <0.00%> (-3.40%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9aa0407...d7974ef. Read the comment docs.

functions[5] to false, // fun compare(hello: String, world: Int)
),
)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a matrix of cases. Given a list of MethodSignatures and a list of function declarations I want to say if it should match or no. And then check all those cases and see that it's correct.

Do you know any better way to write this and no make it a maintainance hell?

Copy link
Member

Choose a reason for hiding this comment

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

What I can think of is to give each functions element a specify name, such as ToStringNoArg, ToStringOneArg, ToStringTwoArgs, CompareNoArg, CompareOneArg, and CompareTwoArgs.

This does prevent magic numbers being used, but the maintenance cost is still there.

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 had that. The problem is that the readability is a bit worst:

ToStringNoArg to false, // fun toString()
ToStringOneArg to false, // fun toString(hello: String)
ToStringTwoArgs to false, // fun toString(hello: String, world: Int)
CompareNoArg to false, // fun compare()
CompareOneArg to false, // fun compare(hello: String)
CompareTwoArgs to false, // fun compare(hello: String, world: Int)

vs

functions[0] to false, // fun toString()
functions[1] to false, // fun toString(hello: String)
functions[2] to false, // fun toString(hello: String, world: Int)
functions[3] to false, // fun compare()
functions[4] to false, // fun compare(hello: String)
functions[5] to false, // fun compare(hello: String, world: Int)

The second one keeps the columns aligned so it's a bit easier to read and modify. Usually I don't care about alignment and I would vote against comments that are far too easy to get outdated but in this case I'm not that sure. Do you think the first is better?

Copy link
Member

Choose a reason for hiding this comment

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

I like the former one. The second has the horizontal alignment but I agree with "Clean code" that it shouldn't matter - Once we have more than 10 items, we do not get columns fully aligned anymore.

Additionally, I don't think the comments become necessary in the former case.

Copy link
Member

@chao2zhang chao2zhang left a comment

Choose a reason for hiding this comment

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

I like FunctionSignature. If we want to emphasize its core functionality, we can call it FunctionMatcher.

functions[5] to false, // fun compare(hello: String, world: Int)
),
)
}
Copy link
Member

Choose a reason for hiding this comment

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

What I can think of is to give each functions element a specify name, such as ToStringNoArg, ToStringOneArg, ToStringTwoArgs, CompareNoArg, CompareOneArg, and CompareTwoArgs.

This does prevent magic numbers being used, but the maintenance cost is still there.

@BraisGabin BraisGabin force-pushed the pre-fix-3855 branch 2 times, most recently from f71d8a5 to 800d094 Compare October 11, 2021 09:15
@chao2zhang chao2zhang added this to the 1.19.0 milestone Oct 13, 2021
@BraisGabin BraisGabin merged commit 795048e into main Nov 6, 2021
@BraisGabin BraisGabin deleted the pre-fix-3855 branch November 6, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants