-
Notifications
You must be signed in to change notification settings - Fork 221
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
paramsNonEmpty returns NonEmptyList[A] #582
Conversation
7262942
to
7f0f5a0
Compare
Current coverage is
|
@@ -337,11 +338,13 @@ trait Endpoints { | |||
* at least one element) multi-value query-string param `name` from the request into a `Seq` or | |||
* raises a [[Error.NotPresent]] exception when the params are missing or empty. | |||
*/ | |||
def paramsNonEmpty(name: String): Endpoint[Seq[String]] = | |||
def paramsNonEmpty(name: String): Endpoint[NonEmptyList[String]] = |
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.
How do you feel about introducing a new function paramsNel
and keeping the old one (behind deprecation)? I want to keep the API unchanged and I think paramsNel
sounds like a reasonable name.
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.
Okay, let's introduce new method
54e4b6f
to
b28454b
Compare
*/ | ||
def paramsNel(name: String): Endpoint[NonEmptyList[String]] = | ||
option(items.ParamItem(name))(requestParams(name)).mapAsync({ | ||
case values if values.exists(_.nonEmpty) => |
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.
Can we simplify that and perform just a single traversal? Something like:
.mapAsync { values => values.filter(_.nonEmpty) match {
case hd :: tl => Future.value(NonEmptyList(hd, tl))
case _ => Future.exception(Error.NotPresent(items.ParamItem(name)))
}}
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.
Emh, but in that case it could contain empty parameters
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.
Ah, I see.
b28454b
to
fdf1b08
Compare
@@ -24,6 +23,10 @@ import shapeless._ | |||
*/ | |||
object Main extends App { | |||
|
|||
(1,2,3) match { |
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.
Some unwanted changes.
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.
I should go sleep :(
fdf1b08
to
98f8b1e
Compare
Thanks @imliar! Merging this! |
As it was discussed earlier: #577 (comment)