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

Calling declared(params) from child namespace fails to include parent namespace defined params #503

Closed
wants to merge 7 commits into from

Conversation

myitcv
Copy link
Contributor

@myitcv myitcv commented Nov 6, 2013

The test in this PR highlights the problem as described in the subject.

The docs seem to suggest that a params block defined against resource in this way should apply to every method within the resource(/namespace)

Hence the expectation that a call to declared(params) should include the param defined against the resource.

Does this make sense? Hopefully the test does!

@myitcv myitcv mentioned this pull request Nov 6, 2013
last_response.status.should == 200
end
end

Copy link
Member

Choose a reason for hiding this comment

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

This is certainly a good way to write a test, but generally I'm trying to put test conditions in the test itself. Mind rewriting it with say something like this:

get do
 {
    params: params,
    declared_params: declared(params)
 }.as_json
end

...

get ...
json = JSON.parse(last_response)
json[:params][:id].should == 123
json[:declared_params].keys.should include :foo # rather than == true, or at least use be_true

Thanks.

@dblock
Copy link
Member

dblock commented Nov 6, 2013

And yes, this totally makes sense.

@myitcv
Copy link
Contributor Author

myitcv commented Nov 7, 2013

Done. Will also write some documentation for declared via #502 given this test describes the desired behaviour.

@dblock
Copy link
Member

dblock commented Nov 7, 2013

Test looks good. There's a rubocop violation (trailing space), so Travis didn't run this (failing) test, fyi. All you have to do now is fix this behavior :)

@myitcv
Copy link
Contributor Author

myitcv commented Nov 7, 2013

Sorry, that's fixed now too.

Will take a look at fixing declared at some point, hopefully soon!

@dblock
Copy link
Member

dblock commented Nov 7, 2013

Btw, if I were to nitpick on the test I would change it to be in JSON format, therefore avoiding having to explicitly call to_json:

    before do
      subject.format :json
      subject.namespace :something do
        resource ':id' do
          ...
          get do
            {
              params: params,
              declared_params: declared(params)
            }
          end
        end
      end
    end

@myitcv
Copy link
Contributor Author

myitcv commented Nov 7, 2013

No, that's fair enough. Worth going to the effort of ensuring new tests set and follow best practice!

@myitcv
Copy link
Contributor Author

myitcv commented Dec 5, 2013

@dblock - this looks to be the correct way to fix the bug, especially given this piece of code that gathers the validations in the same way.

Thoughts?

@dblock
Copy link
Member

dblock commented Dec 7, 2013

Looks good, I'll merge squashed.

@dblock
Copy link
Member

dblock commented Dec 7, 2013

Merged via 535fd7d. Thanks.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 739c131 on myitcv:nested_declared_usage into * on intridea:master*.

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