Feature: Allow Opt for optional params along with Option - #99
Conversation
| let bodyName = bodyArgument.name | ||
| res.add(quote do: | ||
| let `bodyName` {.used.}: Opt[ContentBody] = | ||
| if `bodyParam`.isSome(): |
There was a problem hiding this comment.
why is this game here needed? is bodyParam an Option?
There was a problem hiding this comment.
That's needed because bodyParam is an Option in RestApiCallback and RestΑpiCallback2: https://github.com/status-im/nim-presto/blob/master/presto/route.nim#L22-L33
I could make it a generic type (i.e. RestApiCallback2[T: Option[ContentBody] or Opt[ContentBody] = ... body: T) and all the types and procs that use it as well... I'll see if that's viable.
| when defined(metrics): | ||
| import metrics | ||
|
|
||
| converter toOpt*[T](x: Option[T]): Opt[T] = |
There was a problem hiding this comment.
Converters have global effects well beyond Presto. Not sure this is a bad conversion to have in place as such, but definitely tradeoffs.
There was a problem hiding this comment.
The tradeoff is between modifying a significant portion of the code in Presto or introducing potentially undesired conversions. Making RestApiCallback2 compatible with Opt is totally doable without a converter, it's just that a lot more code will have to change.
TBH the initial solution with converting Option to Opt in processApiCall doesn't look that bad in comparison.
I'm currently trying to bake something without a converter and explicit Option → Opt conversion.
There was a problem hiding this comment.
this converter won't fly for many reasons, chief among them:
- Opt and Option semantically differ in how they handle
ref - they introduce the expensive copy that we're looking to avoid by using a single type throughout
There was a problem hiding this comment.
ie even RestApiCallback3 is the better option here - the code should be generic over Opt/Option so that the whole pipeline is done without extraneous conversions
3 participants
Closes #87