Support named LambdaTypeName parameters #270
Conversation
|
|
||
| constructor(name: String, type: Type) : this(name, type.asTypeName()) | ||
|
|
||
| fun toCodeBlock() = if (name != null) { |
| @JvmStatic fun get( | ||
| receiver: TypeName? = null, | ||
| parameters: List<TypeName> = emptyList(), | ||
| returnType: TypeName) | ||
| = LambdaTypeName(receiver, parameters, returnType) | ||
| namedParameters: List<Parameter> = emptyList(), |
There was a problem hiding this comment.
It's weird to have two lists. I can't mix and match to get like x: Long, y, z: Long.
There was a problem hiding this comment.
Didn't wanna break existing API, and adding a new factory method with parameters: List<Parameter> wouldn't compile due to type erasure. If breaking is fine - I'd just get rid of parameters: List<TypeName>.
There was a problem hiding this comment.
Hmm we can use @JvmName to defeat the erasure 🙈
There was a problem hiding this comment.
Oh yeah, let me try that. Wonder how that would work for JS, no erasure - no problem?
There was a problem hiding this comment.
Well, I couldn't come up with a meaningful value for @JvmName, so I went the following way instead: the version with parameters and namedParameters stays, but is deprecated in favor of the version with parameters: List<Parameter>. We can remove the former when going 1.0. Does this make sense?
There was a problem hiding this comment.
I’m not too concerned with backwards-compatibility at this point.
There was a problem hiding this comment.
Agree. So would a single factory method with parameters: List<ParameterSpec> be enough, given that we'll have versions with vararg parameters: ParameterSpec and vararg parameters: TypeName?
Egorand
force-pushed
the
egorand/named-lambda-params
branch
from
0b75f81 to
d8dbc80
Compare
| @@ -55,18 +55,51 @@ class LambdaTypeName internal constructor( | |||
| } | |||
|
|
|||
| companion object { | |||
| /** Returns a lambda type with `returnType` and parameters of listed in `parameters`. */ | |||
| /* | |||
| import java.lang.reflect.Type | ||
| import kotlin.reflect.KClass | ||
|
|
||
| data class Parameter(val name: String? = null, val type: TypeName) { |
There was a problem hiding this comment.
This competes with an existing class, ParameterSpec. I’m anxious about having both types that mean almost the exact same thing. Would it be simpler to just use ParameterSpec in LambdaTypeName ?
There was a problem hiding this comment.
That's a good point. We'll need only name and type, should we just ignore any other params set on the ParameterSpec?
There was a problem hiding this comment.
Hmm, the problem is that name is mandatory for ParameterSpec, and optional for LambdaTypeName. Will switching from name: String to name: String? on ParameterSpec be justified for that purpose? Then we could have ParameterSpec.ofType() to create an unnamed parameter.
There was a problem hiding this comment.
That sounds a bit weird.
There was a problem hiding this comment.
How about we permit an empty string as the name in those cases, and require a factory method to get instances like that:
ParameterSpec string = ParameterSpec.unnamed(String::class)
|
|
||
| constructor(name: String, type: Type) : this(name, type.asTypeName()) | ||
|
|
||
| internal fun toCodeBlock() = if (name != null) { |
There was a problem hiding this comment.
off topic: would it be better to do this kind of expression without braces?
internal fun toCodeBlock() = if (name != null)
CodeBlock.of("%L: %T", name, type) else
CodeBlock.of("%T", type)
There was a problem hiding this comment.
Yeah, I think that'd be fine with the guidelines
| .receiver(LambdaTypeName.get( | ||
| parameters = listOf( | ||
| Parameter("name", String::class), | ||
| Companion.ofType(Int::class), |
There was a problem hiding this comment.
off topic: referencing Companion like this seems out of place. Idiomatic?
There was a problem hiding this comment.
Nope, there seems to be a problem with auto-complete and I didn't notice it, gonna fix.
| @@ -17,7 +17,7 @@ package com.squareup.kotlinpoet | |||
|
|
|||
| class LambdaTypeName internal constructor( | |||
| val receiver: TypeName? = null, | |||
| parameters: List<Parameter> = emptyList(), | |||
| parameters: List<ParameterSpec> = emptyList(), | |||
There was a problem hiding this comment.
Question: should we have checks to verify that all ParameterSpecs here have only the name and the type and nothing else?
There was a problem hiding this comment.
We should check what's supported. I have a feeling annotations might work...
There was a problem hiding this comment.
Unsupported [annotation on parameter in function type]
There was a problem hiding this comment.
Modifiers and default values are unsupported as well, hence just name and type
| | name: String, | ||
| | Int, | ||
| | age: Long | ||
| |) -> Unit).whatever(): Unit = Unit |
There was a problem hiding this comment.
Formatting here is weird as we currently wrap parameter lists with 2 or more elements. I'll open an issue to improve it.
There was a problem hiding this comment.
Yeah we need a smarter CodeWriter which potentially buffers elements so that they can be rewritten when a line break is required.
Fixes #269