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

upgrades to grape 0.14.x; grape-entity 0.5.x #336

Merged
merged 4 commits into from
Feb 8, 2016

Conversation

LeFnord
Copy link
Member

@LeFnord LeFnord commented Jan 28, 2016

No description provided.

@dblock
Copy link
Member

dblock commented Jan 29, 2016

Thanks. Fix CI please (rubocop -a), squash commits.

@LeFnord LeFnord force-pushed the swagger-2.0 branch 2 times, most recently from 51c8aaa to 729de7b Compare February 1, 2016 19:48
@LeFnord
Copy link
Member Author

LeFnord commented Feb 4, 2016

please merge so that I can work on,

is it possible to cut off my fork and branch from this one, so that I can work on my fork
and make a PR to yours independent from my PRs of feature branches?
so travis don't need to run on every commit, thanks

the failed builds are rubinius, could not find a binary of it
and jruby, has problems with utf-8 chars, but this seems to be a standard java problem, not ruby

include_context "the api entities"

before :all do
module TheApi
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, however these classes will be declared globally, you might want to change it to a dynamic class, like Class.new(Grape::API) or namespace them with something closer to the spec name, like module ContentTypeSpec ....

@dblock
Copy link
Member

dblock commented Feb 5, 2016

This is great, lets try to finish it.

  • We don't need to worry about Rubinius, replace rbx-2.2.10 with rbx-2 and add it to allow_failures.
  • You do need to fix the JRuby build, the problematic file is most likely just missing a UTF-8 marker on top.
  • Please update CHANGELOG and README.
  • Squash your commits.
  • Is this still compatible with grape-entity 0.4.x? What else is changing that needs to be in UPGRADING?

@LeFnord
Copy link
Member Author

LeFnord commented Feb 5, 2016

thanks for the response, I'll try to do it in the next hours …

to your questions:

  • yes it is compatible with grape-entity 0.4.8 and 0.5.0, it was a requirement
  • the README should be up-to-date, have to do some work on the CHANGELOG

a last question: can I change the version to something starting with 0.2.x, to indicate, breaking changes,
and create a tag for that (this is required from my firm, to use it)

@dblock
Copy link
Member

dblock commented Feb 5, 2016

Next version should be 0.11.0, you can update that. Don't worry about tagging, I'll cut a release once we have everything on green.

@LeFnord LeFnord force-pushed the swagger-2.0 branch 8 times, most recently from 58620fc to 9e22931 Compare February 5, 2016 22:56
removes pry

runs under 2.3

updates gems, corrects parameter, which is in array, make rubocop happy

fixes jruby build

changes for TravisCI
@dblock
Copy link
Member

dblock commented Feb 8, 2016

This is looking good. Should I merge this? Squash commits? Update CHANGELOG?

@LeFnord
Copy link
Member Author

LeFnord commented Feb 8, 2016

you can merge it, if you want 😄
the changelog is updated and I squashed some commits, sorry not all

for the next work, I want to merge this work into my fork (there it is master),
and make a regular PR, is it ok to do so?

next work should be:

  • make params related stuff modular
  • make adding of user defined stuff available → support for something like x-foo- …, first for the paths section, later for definition section

is it ok for you?

dblock added a commit that referenced this pull request Feb 8, 2016
upgrades to grape 0.14.x; grape-entity 0.5.x
@dblock dblock merged commit 9aaa2be into ruby-grape:swagger-2.0 Feb 8, 2016
@dblock
Copy link
Member

dblock commented Feb 8, 2016

I merged this one.

I am down with a PR into master that looks like it could actually ship. The biggest concerns for me are UPGRADING, README and CHANGELOG, where people know what they are getting.

@LeFnord
Copy link
Member Author

LeFnord commented Feb 8, 2016

thanks for merging
no, I don't mean into into this master,
I mean, I want work on 'my' master and make regular PRs to the this swagger-2.0 branch

@LeFnord LeFnord deleted the swagger-2.0 branch February 8, 2016 20:35
@LeFnord LeFnord restored the swagger-2.0 branch February 8, 2016 20:37
@LeFnord LeFnord deleted the swagger-2.0 branch February 8, 2016 20:37
@dblock
Copy link
Member

dblock commented Feb 8, 2016

It's your master :)

LeFnord pushed a commit to LeFnord/grape-swagger that referenced this pull request Feb 9, 2019
upgrades to grape 0.14.x; grape-entity 0.5.x
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