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.

Sign up for GitHub

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

Jump to bottom

Add support for grape version cascading #535

Closed
wants to merge 5 commits into from
Closed

Add support for grape version cascading #535

wants to merge 5 commits into from

Conversation

qinix
Copy link

@qinix qinix commented Nov 24, 2016

Support API endpoints like so:

module API::V1
  class Root < Grape::API
    version [:v1, :v2], using: :path

    mount API::V1::Places
    mount API::V1::Users

    add_swagger_documentation api_version: 'v1', mount_path: '/docs'
  end
end

Use the first value in the array as the version component in url.

related issue: #146

LeFnord
LeFnord requested changes Nov 24, 2016
View reviewed changes
lib/grape-swagger/endpoint.rb
@@ -223,7 +223,7 @@ def apply_success_codes(route)
end

def tag_object(route)
Array(route.path.split('{')[0].split('/').reject(&:empty?).delete_if { |i| ((i == route.prefix.to_s) || (i == route.version)) }.first)
Array(route.path.split('{')[0].split('/').reject(&:empty?).delete_if { |i| ((i == route.prefix.to_s) || (i == route.version.to_s) || (route.version.is_a?(Array) && route.version.map(&:to_s).include?(i))) }.first)
Copy link
Member

Choose a reason for hiding this comment

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

think its time to refactor 😉
btw it would make rubucop happy

spec/swagger_v2/namespace_tags_prefix_spec.rb
end
end

class TagApi < Grape::API
prefix :api
mount TheApi::CascadingVersionApi
mount TheApi::NamespaceApi
add_swagger_documentation version: 'v1'
Copy link
Member

Choose a reason for hiding this comment

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

please remove version, or use doc_version instead

@LeFnord
Copy link
Member

LeFnord commented Nov 24, 2016

thanks @qinix for contribution, please my comments above

@qinix
Copy link
Author

qinix commented Nov 24, 2016

updated @LeFnord

LeFnord
LeFnord approved these changes Nov 24, 2016
View reviewed changes
Copy link
Member

@LeFnord LeFnord left a comment

Choose a reason for hiding this comment

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

please fix it

@Overbryd
Copy link

I am very interested in that one. What the status? Should the tests still be breaking?

Just running on the branch @qinix provided, works so far, yet I miss the opportunity to cascade different versions from one documentation to another.

@qinix
Copy link
Author

qinix commented Dec 15, 2016

@Overbryd The tests breaks for grape lower than 0.16. If you are using 0.16 or bigger, it will work for you. There is some strange behavior in low version of grape, I'm not willing to fix it now since I'm not using such old version. I'm using it in production now, you can trust it.

@LeFnord
Copy link
Member

LeFnord commented Dec 15, 2016

@qinix it is understandable, but it would be nice to communicate it … the PR is very welcome

actual I'm preparing ruby 2.4 support, for that I dropped grape support < 0.16.0
it would be nice if can you re-target your PR against this prepare_for_ruby_2.4

@qinix
Copy link
Author

qinix commented Dec 15, 2016 •
edited
Loading

@LeFnord sorry for my poor english...

Okay I will try to re-target it against that.

@LeFnord
Copy link
Member

LeFnord commented Dec 15, 2016

@qinix no problem most people in the world are no native english speaker
… only natives think so 😉

and thanks again

@qinix qinix mentioned this pull request Dec 16, 2016
Closed
@LeFnord
Copy link
Member

LeFnord commented Dec 18, 2016

is included here: https://github.com/LeFnord/grape-swagger/tree/prepare_for_ruby_2.4

@LeFnord LeFnord closed this Dec 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeFnord LeFnord LeFnord approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qinix @LeFnord @Overbryd