Skip to content
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

Provide rule when a default parameter value should be used #12

Closed
arouel opened this issue Jul 12, 2014 · 12 comments
Closed

Provide rule when a default parameter value should be used #12

arouel opened this issue Jul 12, 2014 · 12 comments

Comments

@arouel
Copy link
Contributor

arouel commented Jul 12, 2014

Lets say we have the following query parameters

val start = param[Int]("start", 0)
val limit = param[Int]("limit", 10)

and we want to apply the default values always if a given predicate evaluates true.

val start = param[Int]("start", 0, _ < 0)
val limit = param[Int]("limit", 10, _ < 1)
@arouel
Copy link
Contributor Author

arouel commented Jul 14, 2014

I added a first implementation with commit 35a1f81.

@bryce-anderson
Copy link
Member

I like the feature, but would like to see if we can keep the AST clean by moving the validation to the parsers. The question arises what to do when a value parses, but fails validation? In that case I think we need a ternary representation for the return value. Something like

sealed trait ParseResult[T] {
    def flatMap(... blah blah... ) ...
}
case class ParseFail(error: String) extends ParseResult[Nothing]
case class ValidationFail(error: String) extends ParseResult[Nothing]
case class Success[T](value: T) extends ParseResult[T]

@arouel
Copy link
Contributor Author

arouel commented Jul 16, 2014

Sounds good. We should do that improvement. Than I can also solve other issues more easily.

@bryce-anderson
Copy link
Member

There are two schools of thought concerning handling error in the presence of a default.
1: On failure use the default
2: On failure fail.
Currently we are doing 1, but I like 2 better. I think returning a unknown value can result in unexpected results especially when an interface changes. I've made a branch to do that, particularly commit 1436324. The tests are not all updated to the new result, but I'll get that done soon.
Is there a show stopper with this change?

@bryce-anderson
Copy link
Member

The tests are brought into compliance with commit 7c2bbc7.

@arouel
Copy link
Contributor Author

arouel commented Jul 18, 2014

I'm with you and prefer the 2 over 1. This makes my use cases much easier instead of working with Option everywhere.

@bryce-anderson
Copy link
Member

I feel like this should be closed. If thats not true, feel free to reopen.

@arouel
Copy link
Contributor Author

arouel commented Jul 23, 2014

I thought a bit about the current behavior and have the following question: Do we want to use the default value in case a number is expected but cannot be parsed? Maybe this is more handy?

@bryce-anderson
Copy link
Member

We have the following possibilities when querying for an Int with id foo:

  • /hello/world Use the default.
  • /hello/world?foo=1 Use the result.
  • /hello/world?foo I say use the default as no value was given.
  • /hello/world?foo= I say this is invalid because you've passed an invalid value, "".
  • /hello/world?foo=one Should fail, this is an invalid query. Otherwise I would consider any result spooky.

In summary, I think we should only use the default when a value has not been presented, and never when an invalid value has been presented. The client should be alerted to their mistake so they can fix their behavior, not given a result which they may not have intended to fetch. Are there good reasons for behavior contrary to that above?

@arouel
Copy link
Contributor Author

arouel commented Jul 23, 2014

Make sense to behave as you described it. Maybe we can improve the HTTP response by providing a better status code. A JSON-representation with a message would be nice but should be defined by the user itself.

Okay, now I have it. A user should be able to define a strategy/function in case the value is not parseable. Does it make sense?

@bryce-anderson
Copy link
Member

Yes, a failure strategy would be nice. We should make an issue for that and
start a branch.
On Jul 23, 2014 9:37 AM, "André Rouél" notifications@github.com wrote:

Make sense to behave as you described it. Maybe we can improve the HTTP
response by providing a better status code. A JSON-representation with a
message would be nice but should be defined by the user itself.

Okay, now I have it. A user should be able to define a strategy/function
in case the value is not parseable. Does it make sense?


Reply to this email directly or view it on GitHub
#12 (comment).

@arouel
Copy link
Contributor Author

arouel commented Jul 23, 2014

We work in a new branch for that feature.

@arouel arouel closed this as completed Jul 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants