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

.with and .without accept an array of args as well #107

Closed
dacbd opened this issue Jul 23, 2013 · 6 comments
Closed

.with and .without accept an array of args as well #107

dacbd opened this issue Jul 23, 2013 · 6 comments
Milestone

Comments

@dacbd
Copy link
Contributor

dacbd commented Jul 23, 2013

I made a boo boo and passed an array to these methods instead of calling the method with them as individual parameters, fixed my mistake after realizing I was doing it wrong via the crypt 400 response and second look at the readme, but was wondering if you would be open to that as an acceptable form of input, for example:

validate: {
    payload: {
        latitude: Hapi.types.Number()
            .min(-90).max(90)
            .optional()
            .with(['longitude','radius'])
        longitude: Hapi.types.Number()
            .min(-180).max(180)
            .optional()
            .with(['latitude', 'radius'])
        radius: Hapi.types.Number()
            .min(0)
            .optional()
            .with(['latitude', 'longitude'])
    }
}

instead of:

validate: {
    payload: {
        latitude: Hapi.types.Number()
            .min(-90).max(90)
            .optional()
            .with('longitude','radius')
        longitude: Hapi.types.Number()
            .min(-180).max(180)
            .optional()
            .with('latitude', 'radius')
        radius: Hapi.types.Number()
            .min(0)
            .optional()
            .with('latitude', 'longitude')
    }
}

If you don't like the idea then we can close the issue otherwise I'll send a pull request later.

@geek
Copy link
Member

geek commented Jul 23, 2013

How would .with(['latitude', 'longitude'], 'something') be handled, or multiple arrays as arguments. Instead, can you do a with.apply(n, ['latitude', 'longitude']) if you want to pass in the args as an array.

@dacbd
Copy link
Contributor Author

dacbd commented Jul 23, 2013

var args = Array.isArray(arguments[0]) ? arguments[0].concat(arguments.slice(1) : arguments

Its an idea and by the looks you aren't fond of it, you could argue what if's all day .with( 'something', ['latitude', 'longitude'], something2)
It would either be as it is now, or the current way and with the arguments[0] being an array of what you want, not a combination of the two.
I'll close the issue in a bit incase someone has any other input.

@geek
Copy link
Member

geek commented Jul 23, 2013

I am not opposed to it, but can't think of a good use case. Any thoughts on a valid use case? The other edge case may be with([], [], [], 'something')

@dacbd
Copy link
Contributor Author

dacbd commented Jul 23, 2013

So this isn't really about expanding to a new use case its more of increasing usability. This is quite possibly the wrong approach to the problem I encountered.

When I incorrectly used arrays i received this as a response:

{ code: 400,
  error: 'Bad Request',
  message: 'not valid, not valid, not valid' }

It is my understanding that the .with/.without should only ever have other key names of the object you are validating as input, so those methods are only expecting strings as input. When it received an array it choked and said the payload was invalid when it tried to check and see if the key objectToBeValidated[[]] was present in said object.

maybe a more appropriate action would be to check and make sure arguments only contains strings? and to throw an error with inappropriate use of the method.

calling the method like so: .with([]) would be the same as .with()

If you understand what I'm saying better than we can:

  • implement a single array of args as valid input
  • throw an error for arguments that aren't strings (unless [] {} or 123 is valid for said function) which case you are correct and there isn't a use case
  • change the error message so its more clear what is really happening in this case
  • leave everything the way it is, maybe add an example to the readme of what not todo

Overall you are correct, there isn't a great use case for it. I made the mistake and it wasn't clear what the problem was until I dug a little deeper.

@geek
Copy link
Member

geek commented Jul 23, 2013

Awesome idea, we should throw when the args aren't strings. Do you want to do a PR for this?

@dacbd
Copy link
Contributor Author

dacbd commented Jul 23, 2013

sure.

@geek geek closed this as completed Jul 24, 2013
@lock lock bot locked as resolved and limited conversation to collaborators Jan 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants