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

Fix problem with using prefix name somewhere in api paths #172

Merged
merged 1 commit into from
Nov 13, 2014
Merged

Fix problem with using prefix name somewhere in api paths #172

merged 1 commit into from
Nov 13, 2014

Conversation

grzesiek
Copy link
Contributor

Hi.

I've encountered problem with generating documentation when I used prefix string somewhere is API path.
For example, you can use prefix :api and use

get :apitest do
  "test api"
end

Generated path for this, is, for example, /api/something/apitest.

The problem is when in

route_match = route.route_path.split(route.route_prefix).last.match('\/([\w|-]*?)[\.\/\(]')

route_path is spitted incorrectly because second "api" is also matched.

My solutions changes this code to match only first route_prefix occurrence (greedy RegExp). This is quick solution, and probably needs deeper investigation. It also fixes problem with

grape-swagger-0.8.0/lib/grape-swagger.rb:20:in `split': wrong argument type Symbol (expected Regexp) (TypeError)

what happens when prefix is a Symbol.

Kind regards,
Grzegorz

@dblock
Copy link
Member

dblock commented Nov 12, 2014

This needs a spec and a CHANGELOG entry to be merged, please. Also squash the commits.

@grzesiek
Copy link
Contributor Author

@dblock Added specs, updated changelog, squashed commits

@grzesiek
Copy link
Contributor Author

@dblock
Copy link
Member

dblock commented Nov 13, 2014

Thanks, 👍

dblock added a commit that referenced this pull request Nov 13, 2014
Fix problem with using prefix name somewhere in api paths
@dblock dblock merged commit d4bae9b into ruby-grape:master Nov 13, 2014
@swistaczek
Copy link
Contributor

Good work @grzesiek 👍

@SleeplessByte
Copy link

👍

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.

4 participants