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

Don't require hashie #44

Merged
merged 2 commits into from
Jul 6, 2017
Merged

Don't require hashie #44

merged 2 commits into from
Jul 6, 2017

Conversation

tsuwatch
Copy link
Contributor

@tsuwatch tsuwatch commented Jul 5, 2017

@coveralls
Copy link

coveralls commented Jul 5, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 99.329% when pulling 09c8765 on tsuwatch:fix/hashie into f94eb48 on ruby-grape:master.

@coveralls
Copy link

coveralls commented Jul 5, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 99.329% when pulling a1e01ed on tsuwatch:fix/hashie into f94eb48 on ruby-grape:master.

@coveralls
Copy link

coveralls commented Jul 5, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 6b65bef on tsuwatch:fix/hashie into f94eb48 on ruby-grape:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 6b65bef on tsuwatch:fix/hashie into f94eb48 on ruby-grape:master.

dblock
dblock requested changes Jul 5, 2017
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

You definitely want a CHANGELOG for this as you're dropping support for older rubies.

.travis.yml
@@ -1,6 +1,7 @@
before_install: gem install bundler -v '1.15.0'
Copy link
Member

Choose a reason for hiding this comment

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

Probably should just be gem install bundler, but maybe locking it is better, dunno.

Copy link
Contributor Author

@tsuwatch tsuwatch Jul 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is because bundler v1.15.1 also has a bug. rubygems/bundler#5373 (comment)
Wait till it is released?
(FIY https://travis-ci.org/ruby-grape/grape-rabl/jobs/250298754)

Copy link
Member

Choose a reason for hiding this comment

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

YOLO whatever version works

@coveralls
Copy link

coveralls commented Jul 6, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 6d80eea on tsuwatch:fix/hashie into f94eb48 on ruby-grape:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 6d80eea on tsuwatch:fix/hashie into f94eb48 on ruby-grape:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 100.0% when pulling 6d80eea on tsuwatch:fix/hashie into f94eb48 on ruby-grape:master.

dblock
dblock requested changes Jul 6, 2017
View reviewed changes
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

More cosmetic changes, please.

.travis.yml
@@ -1,6 +1,7 @@
before_install: gem install bundler -v '1.15.0'
Copy link
Member

Choose a reason for hiding this comment

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

YOLO whatever version works

CHANGELOG.md
@@ -1,6 +1,6 @@
#### Next

* Your contribution here.
* Don't require unused hashie and drop support of Ruby 2.0, 2.1
Copy link
Member

Choose a reason for hiding this comment

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

Make these two separate entries, leave your contribution here and make them in the same format as the other lines with a link to the issue and your name, please.

@dblock
Copy link
Member

dblock commented Jul 6, 2017

I'll just merge it and fixup changelog via #45

@dblock dblock merged commit 15613ab into ruby-grape:master Jul 6, 2017
@tsuwatch
Copy link
Contributor Author

tsuwatch commented Jul 7, 2017

I'm sorry for my carelessness.
Thank you for fixing!

@tsuwatch tsuwatch deleted the fix/hashie branch July 7, 2017 03:50
@dblock
Copy link
Member

dblock commented Jul 7, 2017

you did totally great, thanks for contributing @tsuwatch!

@ryanjohns
Copy link

@dblock Any chance you can release a new version that includes this change?

@dblock
Copy link
Member

dblock commented Nov 17, 2017

Done.

@ryanjohns
Copy link

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dblock dblock dblock requested changes

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

Successfully merging this pull request may close these issues.
4 participants
@tsuwatch @coveralls @dblock @ryanjohns