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

Allow passing of Array with brackets (eg. Array[String], Array[Integer]) #455

Merged
merged 10 commits into from
Jun 16, 2016

Conversation

TySpring
Copy link
Contributor

@TySpring TySpring commented Jun 13, 2016

According to docs in Grape, you can pass a parameter of type array with a nested type, like so:

params do
  requires :my_param, type: Array[String], documentation: { param_type: 'body' }
end

(See https://github.com/ruby-grape/grape#custom-types-and-coercions section for example).

Before this PR, if you do this, the swagger JSON that is generated looks like this:

"my_param": {
    "type": "string",
    "description": "this is my param that is supposed to be an array of strings"
}

Now, it looks like this:

"my_param": {
    "type": "array",
    "description": "This is my param that is an array of strings",
    "items": {
        "type": "string"
    }
}

@TySpring TySpring force-pushed the array-of-primitive-types branch from 3c0d9f7 to f9579eb Compare June 13, 2016 23:49
@@ -235,6 +235,11 @@ def parse_request_params(required)
end
end

def param_type_is_array?(param_type)
return false unless param_type
param_type == 'Array' || (param_type[0] == '[' && param_type[-1] == ']' && !param_type.include?(','))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little hacky :) Maybe just a regex? What's the story with the ,?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dblock I could do a regex. The comma is because of a tricky situation. In Grape, you can specify type in 2 similar ways:

  1. type: Array[String], meaning an array of strings, and
  2. type: [String, Integer], meaning a multi-type param of either 'String' or 'Integer'

In the first case, once the options are within the realm of grape-swagger (at least from what I can tell), the Array prefix is stripped, and it just looks like '[String]'. The only way I could think to disambiguate '[String]' (array of type string) from '[String, Integer]' was to look for once vs. multiple items inside brackets, hence the ",". Open to other suggestions, but it's all I could think of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this more explicit. For example:

param_types = param_type.match(/^\[(.*)\]$/)
param_types = param_types[0].split(',') if param_types
# deal with param_types.size == 1
# deal with param_types.size > 1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also make sure to add tests for the multiple types case.

@dblock
Copy link
Member

dblock commented Jun 14, 2016

This is great, thank you. Take a look at my comments, please, mostly minor.

@TySpring
Copy link
Contributor Author

@dblock thanks for taking a look!

I addressed all the comments, except one (I wasn't sure exactly what you meant on moving the if on assignment)

def param_type_is_array?(param_type)
return false unless param_type
param_type == 'Array' || param_type =~ /\[\w+\]\z/
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're parsing this twice, perpetuating the mess we have here and adding to it :) Refactor param_type_is_array into parse_param_type or something like that and return the array key. Nuke the whole possible_key = param.first.to_s.gsub('[', '[][') as well as part of that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dblock … the whole ('[', '[][') junk could be removed by avoiding the introducing of it here endpoint.rb#L224

I had to make a decision, should the name of the parameter indicating if it is a Hash, Array or not, this solution following the rails naming convention for form data

questions is, is it really needed … think removing it, could made many things easier

Copy link
Contributor Author

@TySpring TySpring Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so @LeFnord are you suggesting that the output for nested Arrays in formData would look like address[line1] as opposed to address[][line1] ?

Also @dblock are you saying you want me to remove the method param_type_is_array? altogether, and move the logic into parse_param_type? This seems contradictory to your comment here: #455 (comment).

@LeFnord @dblock Is your general goal just to "clean up the parsing code" without changing logic? Or is it actually to change the output of this parsing logic to remove the added [] for array types?

@dblock
Copy link
Member

dblock commented Jun 15, 2016

Just to loop the loop, I feel pretty strongly about that comment above on parsing, lets clean it up in this PR?

@TySpring TySpring force-pushed the array-of-primitive-types branch from 95d0ec1 to 00cce26 Compare June 16, 2016 01:50
@dblock
Copy link
Member

dblock commented Jun 16, 2016

I'm merging this, @LeFnord might make more comments and we can iterate from here! Thanks @TySpring for hanging in here.

@dblock dblock merged commit a07385f into ruby-grape:master Jun 16, 2016
@TySpring
Copy link
Contributor Author

Alright sounds great, happy to help more if I can. Thanks!

@LeFnord
Copy link
Member

LeFnord commented Jun 16, 2016

@TySpring … for the your work
@dblock … it's fine for me, Inwant to start a discussion, som, I don't have to decide it alone 😉

@adie adie mentioned this pull request Sep 26, 2017
LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants