Make the type in group of required params required (#722) #886
Conversation
| @@ -33,4 +33,5 @@ en: | |||
| at_least_one: 'are missing, at least one parameter must be provided' | |||
| exactly_one: 'are missing, exactly one parameter must be provided' | |||
| all_or_none: 'provide all or none of parameters' | |||
| invalid_group_type: 'group of required parameters must have a specified type of Array or Hash, type provided: %{group_type}' | |||
There was a problem hiding this comment.
I don't think we want to have the type provided in the message, typically error messages should be fixed, and potentially additional data should indicate the source. I would make this just group of required parameters must be Array or Hash.
|
Can you please elaborate why this change for required and not optional params? I don't see the difference. This needs CHANGELOG, either way.. |
|
There is no specific reasoning behind this, other than what I understood from the issue. It seemed to me that it is specifically targeted at required param groups. |
|
Lets step back (and I re-read everything, I realize I totally contradicted myself :)): #722 says that we want an error when the type is not provided so that people aren't confused. Try to do that, but I do think it should be for any group, not just a |
|
Sounds good to me, I'll get to it then. |
jrichter1
force-pushed
the
required_params_type
branch
from
0ca9feb
to
5c6c1be
Compare
|
So I finally got to take a stab at it, what do you think? |
| # check type for optional parameter group | ||
| if attrs && block_given? | ||
| fail Grape::Exceptions::MissingGroupTypeError.new if type.nil? | ||
| fail Grape::Exceptions::UnsupportedGroupTypeError.new if type != Array && type != Hash |
There was a problem hiding this comment.
Maybe more idiomatic to say [Hash, Array].include?(type), more future-proof.
|
I like it, squash the commits, see my minor comment and lets merge this. |
jrichter1
force-pushed
the
required_params_type
branch
from
5c6c1be
to
0bf6fb2
Compare
|
Done and done. Cheers! |
|
I forgot something, this needs a section in UPGRADING, please. Sorry about that. |
jrichter1
force-pushed
the
required_params_type
branch
from
0bf6fb2
to
2e64114
Compare
|
Should be updated now |
Make the type in group of required params required (#722)
|
Merged. |
|
The README.md needs updating to reflect this change. |
|
Right, @jrichter1 could you please do that? |
fix missing type specification in docs, ref #886
Made it so groups with required params will demand a type to be set to Array or Hash, errors otherwise (made a new exception for that).
Covered with some tests, also had to modify several existing tests to support this.