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

Use detail value for notes #286

Merged
merged 2 commits into from
Aug 19, 2015
Merged

Use detail value for notes #286

merged 2 commits into from
Aug 19, 2015

Conversation

rngtng
Copy link
Contributor

@rngtng rngtng commented Aug 18, 2015

Use detail value for notes

This adds support for using the detail value defined within a block passed to desc as notes value exposed in swagger doc. Next I'd deprecate notes and prefer detail as specified by grape instead.

CAUTION: only works on grape version >= 0.12.0

$ GRAPE_VERSION=HEAD bundle exec rspec ./spec/default_api_spec.rb:62

fixes #212

Please have a look at the spec, is this the right format and place to put? I had a hard time to figure out how the specs are structured. pls advise what’s preferred approach..

@rngtng rngtng changed the title Add detail support Use detail value for notes Aug 18, 2015
@@ -21,6 +21,7 @@
* [#266](https://github.com/tim-vandecasteele/grape-swagger/pull/266): Respect primitive mapping on type and format attributes of 1.2 swagger spec - [@frodrigo](https://github.com/frodrigo).
* [#268](https://github.com/tim-vandecasteele/grape-swagger/pull/268): Fixed handling of `type: Array[...]` - [@frodrigo](https://github.com/frodrigo).
* [#284](https://github.com/tim-vandecasteele/grape-swagger/pull/284): Use new params syntax for swagger doc endpoint, fix an issue that `:name` params not recognized by `declared` method -[@calfzhou](https://github.com/calfzhou).
* [#286](https://github.com/tim-vandecasteele/grape-swagger/pull/286): Use `detail` value for `notes` - fix an issue where `detail` value specified in a block passed to `desc` was ignored. -[@rngtng](https://github.com/rngtng).
Copy link
Member

Choose a reason for hiding this comment

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

There's an extra period after ignored. :)

I think these are two separate problems, so there should be two separate lines.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to update README on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, yes a shot note in README would help. I'll add it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, what do you think? I did not mention notes: as its deprecated soon(?) anyway.

@dblock
Copy link
Member

dblock commented Aug 18, 2015

I forgot something, I think we need an entry for https://github.com/tim-vandecasteele/grape-swagger/blob/master/UPGRADING.md. If you were using notes before it will get overwritten by detail now, right? That's what we want, but it should be called out to avoid surprises.

@dblock
Copy link
Member

dblock commented Aug 18, 2015

Next, the build will run against many versions of Grape. It will be failing now, and needs to be fixed somehow, probably with a pending depending on the version of Grape.

@rngtng
Copy link
Contributor Author

rngtng commented Aug 18, 2015

ok sure, are there any plans to drop support for older grape versions? beeing backwards compatible to the last, let's say, three previous version sounds reasonable to me..

@dblock
Copy link
Member

dblock commented Aug 18, 2015

In theory yes, in practice not really. I think we should drop support only if there's a good reason. Interestingly this is the first feature as far as I know. I would be willing to drop support for something major, like https://github.com/tim-vandecasteele/grape-swagger/issues/219

@rngtng
Copy link
Contributor Author

rngtng commented Aug 18, 2015

ok sure fine with me.

@rngtng
Copy link
Contributor Author

rngtng commented Aug 18, 2015

ok added minor updates, mainly README & UPGRADING

@@ -4,7 +4,7 @@ gemspec

case version = ENV['GRAPE_VERSION'] || '~> 0.9.0'
when 'HEAD'
gem 'grape', github: 'intridea/grape'
gem 'grape', github: 'ruby-grape/grape'
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this.

@dblock
Copy link
Member

dblock commented Aug 18, 2015

I don't see UPGRADING.md changes :(

Tobias Bielohlawek added 2 commits August 19, 2015 10:00
This line can be deleted, as the next line overwrites the value anyway. Looks like a classic cope&paste typo
This adds support for using the `detail` value defined within a block passed to `desc` as `notes` value exposed in swagger doc.

CAUTION: only works on grape version > 0.10.0
```
$ GRAPE_VERSION=HEAD bundle exec rspec ./spec/default_api_spec.rb:62
```

fixes #212
@rngtng
Copy link
Contributor Author

rngtng commented Aug 19, 2015

sorry, changes are now pushed.

@dblock
Copy link
Member

dblock commented Aug 19, 2015

Very good, merging.

dblock added a commit that referenced this pull request Aug 19, 2015
@dblock dblock merged commit 6ef288f into ruby-grape:master Aug 19, 2015
@rngtng
Copy link
Contributor Author

rngtng commented Aug 19, 2015

great, thanks!

@calfzhou
Copy link
Contributor

cool!

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.

How to define notes to an operation?
3 participants