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 multi-method routes to append '(.:format)' only once #188

Merged
merged 2 commits into from
Jun 26, 2012

Conversation

kainosnoema
Copy link
Contributor

Because the same path string is used for each loop when the routes are being built, appending the string using << modifies the original, causing multiple '(.:format)' strings to be appended. This fixes the behavior, resulting in the proper path output.

I started with a spec which fails like this before the commit:

1) Grape::API.route should allow for multiple verbs
   Failure/Error: route.route_path.should eql '/abc(.:format)'

     expected: "/abc(.:format)"
          got: "/abc(.:format)(.:format)"

Because the same path string is used for each loop when the routes
are being built, appending the string using `<<` modifies the original,
causing multiple '(.:format)' strings to be appended. This fixes the
behavior, resulting in the proper path output.

Signed-off-by: Evan Owen <kainosnoema@gmail.com>
@dblock
Copy link
Member

dblock commented Jun 26, 2012

Thank you. Would you please be so kind to add a line to the CHANGELOG. I'll merge it then.

@kainosnoema
Copy link
Contributor Author

Done. I've got another fix coming for some issues I've had mounting Grape APIs at paths like this:

mount AccountsAPI => '/accounts'

Currently that only seems to work with non-Grape Rack apps.

@kainosnoema
Copy link
Contributor Author

Sorry, had the wrong issue number with that first commit. Fixed.

@dblock
Copy link
Member

dblock commented Jun 26, 2012

Yes, that mount problem is #60 I believe, would be great if you fixed it!

dblock added a commit that referenced this pull request Jun 26, 2012
Fix multi-method routes to append '(.:format)' only once
@dblock dblock merged commit ad79137 into ruby-grape:master Jun 26, 2012
@dblock
Copy link
Member

dblock commented Jul 21, 2012

@kainosnoema if you have a moment, could you please confirm that this fixes #60?

@kainosnoema
Copy link
Contributor Author

@dblock no, this has nothing to do with #60. @stephencelis and I spent quite a bit of time trying to solve the mounting issue, but got bogged with the odd differences between how Grape treats vanilla Rack apps vs other Grape APIs.

To be bluntly honest, the whole experience has dampened our enthusiasm. Due to several serious issues (including #60), working with Grape has been mostly frustrating so far. Instead of building around the strengths of Rack, it seems to add a lot of unnecessary complexity while trying to add functionality. As an experiment, we've rolled our own framework that has most of the same functionality, but more closely follows Rack conventions. It's currently at ~600 lines (vs Grape's 7k+) and so far seems easier to build on and augment. Through the process we've come up with a few ideas on possible improvements, but they're not quite mature yet. I'll keep you posted.

@dblock
Copy link
Member

dblock commented Jul 23, 2012

Thanks @kainosnoema. Would love improvements to Grape or, if Grape has served as a trial by fire of how not to do something, a competing open-source framework that's better.

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