Default value for enumeratum #1852
Conversation
| @@ -79,7 +79,10 @@ object EndpointTransput { | |||
| schema(_.modifyUnsafe[U](Schema.ModifyCollectionElements)(_.validate(v))) | |||
|
|
|||
| def description(d: String): ThisType[T] = copyWith(codec, info.description(d)) | |||
| def default(d: T): ThisType[T] = copyWith(codec.schema(_.default(d, Some(codec.encode(d)))), info) | |||
| def default(d: T, encoded: Option[Any] = None): ThisType[T] = { | |||
| val e = Some(encoded.getOrElse(codec.encode(d))) | |||
There was a problem hiding this comment.
Should we allow to pass encoded here? It can cause ambiguity if someone passes a value that doest not match the codec encoding
There was a problem hiding this comment.
Without this change I get following compilation error:
[error] /home/mariusz/sml/tapir/core/src/test/scala/sttp/tapir/annotations/DeriveEndpointIOTest.scala:244:40: unknown parameter name: encoded
[error] val derived = EndpointInput.derived[TapirRequestTest8].asInstanceOf[EndpointInput.Query[TapirRequestTest8]]
[error] ^
[error] /home/mariusz/sml/tapir/core/src/test/scala/sttp/tapir/annotations/DeriveEndpointIOTest.scala:246:43: unknown parameter name: encoded
[error] compareTransputs(EndpointInput.derived[TapirRequestTest8], expectedInput) shouldBe true
[error] ^
There was a problem hiding this comment.
Right, we are passing encoded using this method in annotations macro.
There was a problem hiding this comment.
Encoded defaults only have to be passed to schemas, not to inputs (as they have codecs). So I guess @kubinio123 is right, and we should instead opt out of using the encoded parameter on inputs (and document why :) )
There was a problem hiding this comment.
Ok, I'll revert this change.
I just need to find another way to pass the encoded value from the @default annotation to the schema without using this method.
There was a problem hiding this comment.
But the default value is already passed, being automatically encoded to the correct form?
There was a problem hiding this comment.
It turned out that the EndpointIO.default method was used by the macros instead of the Schema.default. I've updated the macros and reverted the change in EndpointIO.
There was a problem hiding this comment.
Reverting to using EndpointIO.default (as suggested in the review).
Adding more tests for macro handling annotation with additional parameter.
| } | ||
|
|
||
| // #1800 | ||
| test("use enum default if more than one") { |
There was a problem hiding this comment.
How about "should use first specified default value"?
| } | ||
|
|
||
| // #1800 | ||
| test("ignore enum default in request body") { |
There was a problem hiding this comment.
How about "should not add default when no encoded value specified"?
| } | ||
|
|
||
| // #1800 | ||
| test("add enum default in request body with given encoded") { |
There was a problem hiding this comment.
So it matches with case above - "should add default when encoded value specified"
|
Looks good ;) But the same has to be done for macro in scala3 - |
|
I've updated Scala 3 macro. |
|
✅ |
| @@ -86,6 +86,15 @@ class CaseClassField[Q <: Quotes, T](using val q: Q, t: Type[T])( | |||
| case _ => report.throwError(s"Cannot extract annotation: @${annSymbol.name}, from field: ${symbol.name}, of type: ${Type.show[T]}") | |||
| } | |||
|
|
|||
| def extractTreeAndOptTreeFromAnnotation(annSymbol: Symbol): Option[(Tree, Tree)] = constructorField.getAnnotation(annSymbol).map { | |||
| case Apply(_, List(t, TypeApply(Select(_, "$lessinit$greater$default$2"), _))) => (t, '{ None }.asTerm) | |||
There was a problem hiding this comment.
out of curiosity, why not just return the second element of the list, which will correspond to the encoded annotation parameter?
There was a problem hiding this comment.
Currently this method is used in AnnotationMacros (and EndpointAnnotationMacros for Scala 2) where the first parameter is required.
There was a problem hiding this comment.
Hm not sure I understand. Do you mean you needed to cover the cases when there are both 1 or 2 parameters if an annotation?
There was a problem hiding this comment.
I think my question is, why wouldn't this work:
def extractTreeAndOptTreeFromAnnotation(annSymbol: Symbol): Option[(Tree, Tree)] = constructorField.getAnnotation(annSymbol).map {
case Apply(_, List(t, u)) => (t, u)
}There was a problem hiding this comment.
Yes, I've added the method to cover the cases with 1 or 2 parameters. I've described all these cases in a new test-case.
Actually, only the first parameter is currently used, the second one being ignored. So as an alternative I've been considering a different method - returning first arg and simply ignoring the rest.
I've created another PR with this alternative. The patter matching there is indeed much simpler.
There was a problem hiding this comment.
I think my question is, why wouldn't this work: (...)
I wanted to make sure, that the second parameter was Optional. Now I think I might have overcomplicated it :/
Related to issue #1800.
In case of
@defaultannotation the issue is challenging to resolve in general case. Here we lack direct access to codec for given type of request body (containing enum or other complex object).In this PR I'd like to propose some work-aroungs.
First work-around is to add a second field to the
@defaultannotation that would allow to explicitly provide the encoded value: