Dynamic declared class #1631
Dynamic declared class #1631
Conversation
|
Makes sense, but needs a test/changelog/etc., please. |
CHANGELOG.md
Outdated
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| #### Fixes | |||
|
|
|||
| * [#1631](https://github.com/ruby-grape/grape/pull/1631): Dynamic declared class - [@thogg4](https://github.com/thogg4). | |||
There was a problem hiding this comment.
Maybe something more useful that explains the behavior change? Like declared now returns ...
spec/grape/endpoint_spec.rb
Outdated
| '' | ||
| end | ||
|
|
||
| puts d |
There was a problem hiding this comment.
I think there is no need in that piece of code, and the same for above (lines 316, 335)
spec/grape/endpoint_spec.rb
Outdated
| optional :second, default: 'second-default' | ||
| end | ||
| subject.get '/declared' do | ||
| d = declared(params, include_missing: true) |
There was a problem hiding this comment.
Since this is an API, you should return the declared output and compare that, don't use a variable out of scope, it works but is hard to follow and is a hack we stay away from.
There was a problem hiding this comment.
@dblock what is returned by the api call is always a json string. What we are testing here is what is returned by declared inside of the endpoint. Is there a better place to put this to test just that?
There was a problem hiding this comment.
Have the API return d.class as JSON or a string and compare the result.
There was a problem hiding this comment.
@dblock duh, im not sure why i didn't think of that. thanks 😄
spec/grape/endpoint_spec.rb
Outdated
| @@ -298,6 +298,65 @@ def app | |||
| end | |||
| end | |||
|
|
|||
| context 'when params are not built with default class' do | |||
| it 'should return an object that corresponds with the params class - hash with indifferent access' do | |||
There was a problem hiding this comment.
Instead of "should return", just say "returns"
spec/grape/endpoint_spec.rb
Outdated
| subject.params do | ||
| build_with Grape::Extensions::ActiveSupport::HashWithIndifferentAccess::ParamBuilder | ||
|
|
||
| requires :first |
There was a problem hiding this comment.
The actual parameters are not material for the tests, remove them. Make this as short as possible.
|
Looks like there's a legit failure with one of the builds, check it out until this build is green please. |
|
This looks good, just needs a rebase/squash please. |
thogg4
force-pushed
the
dynamic-declared-class
branch
2 times, most recently
from
01e4e40
to
57a1f2e
Compare
thogg4
force-pushed
the
dynamic-declared-class
branch
from
9ec10ce
to
e779593
Compare
#1610 causes the params to be passed to
declaredin the correct class, but once they get there that method only returns aHash