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

Add parameter alias #1674

Merged
merged 1 commit into from
Aug 9, 2017
Merged

Add parameter alias #1674

merged 1 commit into from
Aug 9, 2017

Conversation

glaucocustodio
Copy link
Contributor

@glaucocustodio glaucocustodio commented Aug 7, 2017

Closes #1673.

Please, check if it's ok.

The methods requires and optional have a if opts[:using] (grape/dsl/parameters.rb) but I don't know how this condition can be true, I also didn't find any mention to using on readme.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I find it rather strange that declared(params) needs to be used to enable the alias. I feel like when I use as, params should just be the target structure and maybe there should be some other way to access the "raw" parameters. That seems more predictable, no?

README.md Outdated
@@ -1094,6 +1095,23 @@ params do
end
```

### Alias

It's possible to set alias for parameters using `as` (it can be useful when refactoring an existing API):
Copy link
Member

Choose a reason for hiding this comment

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

You can set an alias for parameters using as, which can be useful when refactoring existing APIs.

README.md Outdated
end
```

The value passed to `as` will be the key when calling `declared(params)` (calling only `params` will not set the alias).
Copy link
Member

Choose a reason for hiding this comment

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

Maybe ", but params will not use the alias."

@@ -115,10 +115,15 @@ def required?

# Adds a parameter declaration to our list of validations.
# @param attrs [Array] (see Grape::DSL::Parameters#requires)
def push_declared_params(attrs)
def push_declared_params(attrs, param_alias = nil)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of param_alias I would make it opts and a Hash of different options, starting with as, for future proofing.

@glaucocustodio
Copy link
Contributor Author

I am not sure if would be cool to use alias with params, because today it seems that params is set directly from rack (@params = @request.params) and we would have to create another method (or an option for params) to get the raw parameters.

@glaucocustodio
Copy link
Contributor Author

I feel like when I use as, params should just be the target structure and maybe there should be some other way to access the "raw" parameters. That seems more predictable, no?

When we use coerce_with: the value is coerced when calling params as well, so I took a look in the implementation and achieved the same for alias, now if you call params the value of as will override the original key. You can check the tests @dblock.

@dblock
Copy link
Member

dblock commented Aug 9, 2017

So much better! Merging.

@dblock dblock merged commit af45b1d into ruby-grape:master Aug 9, 2017
@dblock
Copy link
Member

dblock commented Aug 9, 2017

Nice work @glaucocustodio.

@glaucocustodio glaucocustodio deleted the implement-as branch August 9, 2017 17:19
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