-
-
Notifications
You must be signed in to change notification settings - Fork 768
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
Implements #643 - MagicNumber ignores default values in ctor properties #644
Conversation
@@ -142,6 +146,11 @@ class MagicNumber(config: Config = Config.empty) : Rule(config) { | |||
private fun KtConstantExpression.isPartOfFunctionReturnConstant() = | |||
parent is KtNamedFunction || (parent is KtReturnExpression && parent.parent.children.size == 1) | |||
|
|||
private fun KtConstantExpression.isPartOfConstructor(): Boolean { | |||
val isInConstructor = parent.parent.parent is KtPrimaryConstructor || parent.parent.parent is KtSecondaryConstructor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract parent.parent.parent
and give it a name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have any suggestions?
great-grandparent does not improve the readability much I think.
@@ -142,6 +146,11 @@ class MagicNumber(config: Config = Config.empty) : Rule(config) { | |||
private fun KtConstantExpression.isPartOfFunctionReturnConstant() = | |||
parent is KtNamedFunction || (parent is KtReturnExpression && parent.parent.children.size == 1) | |||
|
|||
private fun KtConstantExpression.isPartOfConstructor(): Boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isPartOfConstructor
sounds like this would also ignore instances where the constructor is called with a magic number: SomeClass(10)
which is not the case, I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. I’ll have a look later on today. Shouldn’t detekt already have such a test case?
Implements #643