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

Patch to handle Array Declaration #237 #268

Merged
merged 1 commit into from
Jul 3, 2015
Merged

Conversation

frodrigo
Copy link
Contributor

Patch to handle Array Declaration #237

@@ -47,6 +47,12 @@ def parse_params(params, path, method)
'double'
when 'Symbol'
'string'
when /^\[(.*)\]$/
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use a named field here, like type. That avoids having a last_match[1] and is probably clearer.

@frodrigo
Copy link
Contributor Author

frodrigo commented Jul 1, 2015

If it's OK I can squash them.

when /^\[(?<type>.*)\]$/
items[:type] = Regexp.last_match[:type].downcase
if PRIMITIVE_MAPPINGS.key?(items[:type])
items[:type], items[:format] = PRIMITIVE_MAPPINGS[items[:type]]
Copy link
Member

Choose a reason for hiding this comment

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

I re-read this and find it a bit strange (or at least looks like a side-effect) that we're assigning items[] inside the case statement. Feels like this logic belongs below where we deal with items? I am not sure though, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Later we deal with parsed_params[:type], not items[:type], items just affected into parsed_params[:items].

@dblock
Copy link
Member

dblock commented Jul 2, 2015

Please squash and update CHANGELOG. Thanks.

@frodrigo
Copy link
Contributor Author

frodrigo commented Jul 2, 2015

Squashed + changelog done

dblock added a commit that referenced this pull request Jul 3, 2015
Patch to handle Array Declaration #237
@dblock dblock merged commit d84b9f7 into ruby-grape:master Jul 3, 2015
@dblock
Copy link
Member

dblock commented Jul 3, 2015

Merged.

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.

2 participants