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

KT-23445 Inspection and quickfix to replace `assertTrue(a == b)` with `assertEquals(a, b)` #1764

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@tommykw
Copy link
Contributor

tommykw commented Jul 16, 2018

}

private fun KtCallExpression.replaceableAssertion(): String? {
val assertions = setOf("assertTrue", "assertFalse")

This comment has been minimized.

@mglukhikh

mglukhikh Jul 23, 2018

Contributor

It's better to make assertions property of a companion object.


override fun fixText(element: KtCallExpression): String {
val assertion = element.replaceableAssertion() ?: return defaultFixText
return "Replace with $assertion"

This comment has been minimized.

@mglukhikh

mglukhikh Jul 23, 2018

Contributor

Take assertion in quotes here please, like Replace with 'assertEquals'

val binaryExpression = valueArguments.first().getArgumentExpression() as? KtBinaryExpression ?: return null
val operationToken = binaryExpression.operationToken

return when (referencedName) {

This comment has been minimized.

@mglukhikh

mglukhikh Jul 23, 2018

Contributor

I'd implement this as a map from pair of <oldName, operationToken> to newName.

@mglukhikh mglukhikh self-assigned this Jul 23, 2018

@tommykw tommykw force-pushed the tommykw:KT-23445 branch from 2159a86 to 402504d Aug 4, 2018

@tommykw

This comment has been minimized.

Copy link
Contributor

tommykw commented Aug 4, 2018

Thank you for your review. Could you review this, again?

@tommykw tommykw force-pushed the tommykw:KT-23445 branch from 402504d to 65f3f3d Aug 4, 2018

@tommykw tommykw force-pushed the tommykw:KT-23445 branch from 65f3f3d to 8cb2583 Aug 4, 2018

}

private fun KtCallExpression.replaceableAssertion(): String? {
val assertions = setOf("assertTrue", "assertFalse")

This comment has been minimized.

@NKb03

NKb03 Aug 4, 2018

Could be factored out as private companion object constant. Probably performance and space cost is negligible but it would look a bit cleaner for me.

private fun KtCallExpression.replaceableAssertion(): String? {
val assertions = setOf("assertTrue", "assertFalse")
val referencedName = (calleeExpression as? KtNameReferenceExpression)?.getReferencedName() ?: return null
if (!assertions.contains(referencedName)) {

This comment has been minimized.

@NKb03

NKb03 Aug 4, 2018

Maybe replace with !in operator

else -> null
}
else -> null
}

This comment has been minimized.

@NKb03

NKb03 Aug 4, 2018

Lines 63-75 could be factored out to private method replacingAssertionMethod with the reference name and the operator token as parameters

var result: String? = null
val referencedName = (calleeExpression as? KtNameReferenceExpression)?.getReferencedName() ?: return result
if (!assertions.contains(referencedName)) {
return result

This comment has been minimized.

@NKb03

NKb03 Aug 4, 2018

Always null, why not simply return null? This could also eliminate the result variable

}

if (getCallableDescriptor()?.containingDeclaration?.fqNameSafe != FqName("kotlin.test")) {
return result

This comment has been minimized.

@NKb03

NKb03 Aug 4, 2018

Same as above, simply return null

result = it.value
return@forEach
}
}

This comment has been minimized.

@NKb03

NKb03 Aug 4, 2018

Replace with
return assertionMap[Pair(referenceName, operationToken)]

@tommykw

This comment has been minimized.

Copy link
Contributor

tommykw commented Aug 5, 2018

Done. Thank you for pointing them.

@NKb03

NKb03 approved these changes Aug 5, 2018

@mglukhikh

This comment has been minimized.

Copy link
Contributor

mglukhikh commented Aug 14, 2018

Merged. I increased level of this inspection to weak warning. Thank you.

@mglukhikh mglukhikh closed this Aug 14, 2018

@tommykw tommykw deleted the tommykw:KT-23445 branch Aug 14, 2018

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