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

specs for declared(params) inside route_param bug. #1430

Merged
merged 1 commit into from
Jun 29, 2016

Conversation

Arkanain
Copy link
Contributor

@Arkanain Arkanain commented Jun 16, 2016

I check some specs on my project and I find out that this functionality work little bit wrong with declared when we have several route_param in one namespace.
For example we have:

class Artists
  namespace :artists do
    route_param :id, type: Integer do
      get do
        declared(params)
      end

      params do
        requires :filter, type: String
      end
      get :some_route do
        declared(params)
      end
    end
    route_param :artist_id, type: Integer do
      mount Compositions
    end
  end
end

declared(params) should return us next hash {id: 1}, but it return hash {id: 1, artist_id: nil}.
Also in first route_param in second endpoint(get :some_route) when we call declared(params) we have only filter params in returned hash {filter: 'some_filter'}

P.S.: I find out where we have problem! Problem is in options for route_param. where I use route_param :id without type: Integer everything is ok.

@Arkanain
Copy link
Contributor Author

@dblock @namusyaka please review this.

@dblock
Copy link
Member

dblock commented Jun 17, 2016

Well these specs pass... weren't they supposed to fail?

@Arkanain
Copy link
Contributor Author

Arkanain commented Jun 17, 2016

@dblock Hmm sorry I missed up. I wrote regression specs. I will rewrite them.

@Arkanain
Copy link
Contributor Author

Arkanain commented Jun 17, 2016

@dblock I changed specs. not it failed and show problem correct.

@Arkanain
Copy link
Contributor Author

Hi guys. Any news about this issue?

@Arkanain
Copy link
Contributor Author

Arkanain commented Jun 28, 2016

@dblock @namusyaka Please review my code. I find out where was a problem and fixed it. If everything ok can you please merge it, because I need this commit in my project.

@Arkanain Arkanain force-pushed the multiple_route_params branch 2 times, most recently from c8aa27e to 2782d4c Compare June 28, 2016 22:16
@namusyaka
Copy link
Contributor

Looks good to me.
@dblock Any thoughts on this?

@dblock
Copy link
Member

dblock commented Jun 29, 2016

Looks good, needs a CHANGELOG entry please.

@Arkanain
Copy link
Contributor Author

@dblock Done.

@@ -11,6 +11,7 @@

#### Fixes

* [#1430](https://github.com/ruby-grape/grape/pull/1430): Fix for declared(params) inside route_param. - [@Arkanain](https://github.com/Arkanain).
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be a pain. There shouldn't be a period after route_param, also maybe quote the code as below?

@Arkanain
Copy link
Contributor Author

@dblock update.

@dblock
Copy link
Member

dblock commented Jun 29, 2016

Excellent, merging.

@dblock dblock merged commit c059f98 into ruby-grape:master Jun 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants