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
Clean up warnings in KotlinJsonAdapter #899
Conversation
Didn't use it on a couple other places where readability would suffer
IntelliJ thinks this is a better approach
@@ -186,18 +189,18 @@ class KotlinJsonAdapterFactory : JsonAdapter.Factory { | |||
// Fall back to a reflective adapter when the generated adapter is not found. | |||
} | |||
|
|||
if (rawType.isLocalClass) { | |||
throw IllegalArgumentException("Cannot serialize local class or object expression ${rawType.name}") | |||
require(!rawType.isLocalClass) { |
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.
Love this! Is this new in 1.3.50? I just discovered it the other day.
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.
The inspection is new, yes
if (binding == null && !parameter.isOptional) { | ||
throw IllegalArgumentException("No property for required constructor ${parameter}") | ||
} | ||
require(!(binding == null && !parameter.isOptional)) { "No property for required constructor $parameter" } |
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.
require(!(binding == null && !parameter.isOptional)) { "No property for required constructor $parameter" } | |
require(binding != null || parameter.isOptional) { "No property for required constructor $parameter" } |
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.
Will fix in a follow-up, didn't know you were doing a pass before merging 😬
@@ -210,13 +213,15 @@ class KotlinJsonAdapterFactory : JsonAdapter.Factory { | |||
val parameter = parametersByName[property.name] | |||
|
|||
if (Modifier.isTransient(property.javaField?.modifiers ?: 0)) { | |||
@Suppress("ReplaceGuardClauseWithFunctionCall") // More readable this way |
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.
Down below you replaced a similar one
if (parameter != null && !parameter.isOptional) { | ||
throw IllegalArgumentException( | ||
"No default value for transient constructor $parameter") | ||
} | ||
continue | ||
} | ||
|
||
@Suppress("ReplaceGuardClauseWithFunctionCall") // More readable this way |
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.
Isn't it just parameter == null || parameter.type == property.returnType
? That's not bad...
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.
Not quite - the null smartcast no longer works when this is done too. On mobile but in one of these I think it was fine, but in the other it meant nullchecking it's mention in the message
Followup fixes from the previous PR
Misc idiomatic cleanups, nothing functional changing