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

Fixed camel case issue #457 #458

Merged
merged 3 commits into from
Jun 15, 2016
Merged

Fixed camel case issue #457 #458

merged 3 commits into from
Jun 15, 2016

Conversation

rayko
Copy link
Contributor

@rayko rayko commented Jun 15, 2016

Issue with camel case endpoints #457

rayko referenced this pull request in gekola/grape-swagger Jun 15, 2016
@@ -13,6 +13,7 @@
* [#444](https://github.com/ruby-grape/grape-swagger//pull/444): Default value provided in the documentation hash, override the grape default [@scauglog](https://github.com/scauglog).
* [#443](https://github.com/ruby-grape/grape-swagger/issues/443): Type provided in the documentation hash, override the grape type [@scauglog](https://github.com/scauglog).
* [#454](https://github.com/ruby-grape/grape-swagger/pull/454): Include documented Hashes in documentation output - [@aschuster3](https://github.com/aschuster3).
* [#457](https://github.com/ruby-grape/grape-swagger/issues/457): Issue with camel case endpoints - [@rayko](https://github.com/rayko/).
Copy link
Member

Choose a reason for hiding this comment

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

Describe what the issue is (something like "camel case endpoint causes ...").

@dblock
Copy link
Member

dblock commented Jun 15, 2016

The build has failed, please check it out.

@rayko
Copy link
Contributor Author

rayko commented Jun 15, 2016

Is that better?

There was a failing spec before I put my hands on it: spec/swagger_v2/api_swagger_v2_detail_spec.rb:144

Tried with several ruby versions including the one used in the builder, just in case, but it still fails. I don't know what might be causing it, though it's just a \n missing on a returned value.

@@ -65,7 +65,6 @@ def combine_routes(app, doc_klass)
next unless route_match
resource = route_match.captures.first
next if resource.empty?
resource.downcase!
Copy link
Member

Choose a reason for hiding this comment

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

maybe you can re-use this method operation_id.rb#L13

@LeFnord
Copy link
Member

LeFnord commented Jun 15, 2016

@Bugagazavr …removed bug label, cause it isn't one, by using ruby naming conventions 😉

@dblock
Copy link
Member

dblock commented Jun 15, 2016

Is that spec failing on HEAD?

@dblock
Copy link
Member

dblock commented Jun 15, 2016

Looks like a newer version of kramdown/rouge is generating slightly different text. Go ahead and just fix the spec (change the \n in there) and make the build green and we can merge.

@rayko
Copy link
Contributor Author

rayko commented Jun 15, 2016

Should I include a comment in the changelog about that? Also, according to @LeFnord this is not a bug anymore, so my previous update on the changelog might need to be moved to the feature section instead of leaving it in the fixes section.

@dblock
Copy link
Member

dblock commented Jun 15, 2016

No need to changelog spec fixes.

Is this really not a bug? Remove your fix, does the spec pass? If so it just doesn't need a CHANGELOG, but we should keep the spec.

@rayko
Copy link
Contributor Author

rayko commented Jun 15, 2016

It's green now. Rebel spec aside, this addresses the issue I've opened earlier, but the labels were changed, so I was asking about that, I don't handle the labels for this. It is an issue because of that downcase method, but it can also be a new feature to support camel case, depends on who looks at this. Either way, without removing that downcase, anyone using camel case will get an error as I've explained in the issue.

@dblock
Copy link
Member

dblock commented Jun 15, 2016

Excellent. I am merging this, it seems straightforward, cc: @LeFnord

@dblock dblock merged commit 4e2c235 into ruby-grape:master Jun 15, 2016
@LeFnord
Copy link
Member

LeFnord commented Jun 15, 2016

👍 … and thanks again

@rayko
Copy link
Contributor Author

rayko commented Jun 15, 2016

Thanks for the attention 😄

LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
* Fixed camel case issue ruby-grape#457

* Description of fixed issue in CHANGELOG.md

* Fixing api_swagger_v2_detail_spec.rb with updated expectation
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.

4 participants