Skip to content

Freeze rubocop version for stable CI #278

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
Merged
merged 5 commits into from
Oct 31, 2017
Merged

Freeze rubocop version for stable CI #278

merged 5 commits into from
Oct 31, 2017

Conversation

kachick
Copy link
Contributor

@kachick kachick commented Oct 31, 2017

@grape-bot
Copy link

grape-bot commented Oct 31, 2017 •
edited
Loading

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.
1 Message
📖 We really appreciate pull requests that demonstrate issues, even without a fix. That said, the next step is to try and fix the failing tests!

Here's an example of a CHANGELOG.md entry:

* [#278](https://github.com/ruby-grape/grape-entity/pull/278): Freeze rubocop version for stable ci - [@kachick](https://github.com/kachick).

Generated by 🚫 danger

@coveralls
Copy link

coveralls commented Oct 31, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 92.72% when pulling a1fcb44 on kachick:fix-rubocop into 6a384cd on ruby-grape:master.

@LeFnord
Copy link
Member

LeFnord commented Oct 31, 2017

thanks @kachick … did you tested it with #277 and it is green?

LeFnord
LeFnord reviewed Oct 31, 2017
View reviewed changes
grape-entity.gemspec Outdated
@@ -23,7 +23,7 @@ Gem::Specification.new do |s|

s.add_development_dependency 'bundler'
s.add_development_dependency 'rake'
s.add_development_dependency 'rubocop', '~> 0.48'
s.add_development_dependency 'rubocop', '~> 0.48.0'
Copy link
Member

Choose a reason for hiding this comment

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

please also add require: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we need to add the line into Gemfile instead of gemspec, I think 🤔

Could you check 8895a69 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And added c362d74 🙇

Copy link
Member

@LeFnord LeFnord Oct 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

think you can remove this line

@LeFnord LeFnord mentioned this pull request Oct 31, 2017
Merged
@kachick kachick mentioned this pull request Oct 31, 2017
Closed
@coveralls
Copy link

coveralls commented Oct 31, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 92.72% when pulling 8895a69 on kachick:fix-rubocop into 6a384cd on ruby-grape:master.

LeFnord
LeFnord reviewed Oct 31, 2017
View reviewed changes
Gemfile Outdated
@@ -4,7 +4,7 @@ source 'http://rubygems.org'

gemspec

group :development do
group :development, :test do
Copy link
Member

Choose a reason for hiding this comment

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

it adds always 0.51 → maybe you have to remove the line in gem spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it adds always 0.51

Yes, How about b66105f?

LeFnord
LeFnord reviewed Oct 31, 2017
View reviewed changes
Gemfile
@@ -4,6 +4,10 @@ source 'http://rubygems.org'

gemspec

group :development, :test do
gem 'rubocop', '~> 0.48.0', require: false
Copy link
Member

Choose a reason for hiding this comment

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

👍

grape-entity.gemspec Outdated
@@ -23,7 +23,7 @@ Gem::Specification.new do |s|

s.add_development_dependency 'bundler'
s.add_development_dependency 'rake'
s.add_development_dependency 'rubocop', '~> 0.48'
s.add_development_dependency 'rubocop', '~> 0.48.0'
Copy link
Member

@LeFnord LeFnord Oct 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

think you can remove this line

@kachick
Copy link
Contributor Author

kachick commented Oct 31, 2017

#278 (comment)

did you tested it with #277 and it is green?

Yes, I tested in my local. But we added some changes. So I'll ensure in #279 💪

@coveralls
Copy link

coveralls commented Oct 31, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 92.72% when pulling c362d74 on kachick:fix-rubocop into 6a384cd on ruby-grape:master.

@LeFnord
Copy link
Member

LeFnord commented Oct 31, 2017

I see … cool 👍

@LeFnord
Copy link
Member

LeFnord commented Oct 31, 2017

please can you close the other PRs, think here is all in 😉

@kachick
Copy link
Contributor Author

kachick commented Oct 31, 2017

Thank you! Then, Can I close #279 ? ⏹

@coveralls
Copy link

coveralls commented Oct 31, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 92.72% when pulling b66105f on kachick:fix-rubocop into 6a384cd on ruby-grape:master.

@coveralls
Copy link

coveralls commented Oct 31, 2017 •
edited
Loading

Coverage Status

Coverage increased (+1.8%) to 94.538% when pulling fa4081a on kachick:fix-rubocop into 6a384cd on ruby-grape:master.

@kachick kachick mentioned this pull request Oct 31, 2017
Closed
@LeFnord LeFnord merged commit 030d02e into ruby-grape:master Oct 31, 2017
@kachick
Copy link
Contributor Author

kachick commented Oct 31, 2017

Oh sorry to misunderstood 🙇

And Thank you for the review! I'll update rest PRs 💪

@kachick kachick deleted the fix-rubocop branch October 31, 2017 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeFnord LeFnord LeFnord left review comments

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
@kachick @grape-bot @coveralls @LeFnord