Skip to content

support for Grape 0.10 with refactored settings #24

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

Merged
merged 1 commit into from
Jan 13, 2015

Conversation

mmclead
Copy link

@mmclead mmclead commented Jan 9, 2015

This pull request enables support for Grape 0.10 since they refactored the way settings are set and retrieved. Also added Appraisals gem to test both versions locally and on travis-CI.

If you can find a prettier way of dealing with this please comment or update.

Can run specs against Grape 0.10 and 0.9 with:

bundle exec appraisal rspec spec

@davidcelis
Copy link
Owner

Thanks for the PR! This solution is good, but I'm not sure if I want to deal with divergent code paths in a single version... I wonder if it would be a better idea to simply change the implementation of Grape and bump api-pagination's major version since it's a breaking change? I can maintain a branch with the current major version, leave Grape's implementation, and continue to merge in bug fixes as they come in.

Also, looking over Grape's changes to settings, it seems like there are a lot of options now... namespace_inheritable, namespace_setting, namespace_stackable, route_setting... My guess is that the type of setting we actually want is route_setting as we're setting pagination info on a single endpoint as opposed to an entire namespace. Would you agree?

@mmclead
Copy link
Author

mmclead commented Jan 12, 2015

It does look like route_setting is the more appropriate setting for this. Moving a major version makes sense to me if you don't want to have the version checking inside the code. Easier to maintain that way too.
I'll update my PR to use route_setting and set the .gemspec to specify grape >= 0.10.1.
I'll leave setting the version of this gem to you after merging.

@davidcelis
Copy link
Owner

Thanks a bunch!

@@ -17,7 +17,7 @@ Gem::Specification.new do |s|
s.require_paths = ['lib']

s.add_development_dependency 'rspec', '~> 3.0'
s.add_development_dependency 'grape'
s.add_development_dependency 'grape', '>= 0.10.1'
Copy link
Owner

Choose a reason for hiding this comment

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

Actually should this be '>= 0.10.0'? Or did the breaking change come in 0.10.1?

Copy link
Author

Choose a reason for hiding this comment

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

0.10.0. good catch

davidcelis added a commit that referenced this pull request Jan 13, 2015
support for Grape 0.10 with refactored settings
@davidcelis davidcelis merged commit 10e4fbc into davidcelis:master Jan 13, 2015
@coveralls
Copy link

coveralls commented Jul 31, 2016

Coverage Status

Changes Unknown when pulling 34879e8 on mmclead:master into * on davidcelis: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