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

Bump dependencies #87

Merged
merged 4 commits into from Dec 22, 2016

Conversation

Projects
None yet
4 participants
@pointlessone
Copy link
Contributor

commented Dec 21, 2016

  • Switch to official docker Ruby image
  • Bump Ruby to 2.3.3
  • Bump Rubocop to 0.46.0
  • Bump other gems as well
@@ -1,3 +1,5 @@
RuboCop::Cop::Bundler::DuplicatedGem

This comment has been minimized.

Copy link
@maxjacobson

maxjacobson Dec 21, 2016

Member

It looks like these new cops are documented in the rubocop codebase -- any idea why the scraper wasn't able to pull them in?

This comment has been minimized.

Copy link
@pointlessone

pointlessone Dec 21, 2016

Author Contributor

TBH, I just forgot to run the scrapper task. Fixed now.

This comment has been minimized.

Copy link
@maxjacobson
@@ -1,3 +1,5 @@
RuboCop::Cop::Bundler::DuplicatedGem
RuboCop::Cop::Bundler::OrderedGems

This comment has been minimized.

Copy link
@maxjacobson

maxjacobson Dec 21, 2016

Member

the introduction of a new namespace should be fine - by default these will be categorized as "style". I think that probably sounds right... WDYT?

@maxjacobson

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

Looking into why the tests didn't run for this PR

@maxjacobson

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

Build forked pull requests was set to off -- I believe it's because this repo was private and was later open sourced. It's fine to set that to on, and I have. Will you push again to trigger a build?

@pointlessone pointlessone force-pushed the pointlessone:dep-bump branch from 0e1bf62 to 667dd65 Dec 21, 2016

@maxjacobson
Copy link
Member

left a comment

Overall the changes look good, but the build is failing and I'm not sure why at the moment. I wonder if it's the changing of the base image?

pointlessone added some commits Dec 21, 2016

@pointlessone pointlessone force-pushed the pointlessone:dep-bump branch from 667dd65 to ffcaa04 Dec 21, 2016

@pointlessone

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2016

Yes, the build failed because of base image change. Weird enough, the engine runs just fine it's just the way specs are invoked revealed the issue.

Updated the Docekrfile.

@maxjacobson

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

Cool, fix looks good.

The one remaining thought I have is that, while we're updating this engine, I think it might be good to check our default .rubocop.yml configuration. We disable all cops, and then enable specific cops that we think are going to be useful to the most number of people. Do you think any of these new cops fit that criteria?

@@ -1,12 +1,12 @@
FROM codeclimate/alpine-ruby:b38
FROM ruby:2.3-alpine

This comment has been minimized.

Copy link
@gdiggs

gdiggs Dec 21, 2016

Contributor

One thing we're conscious of is the size of the images. What does this do to the size of this engine's Docker image?

This comment has been minimized.

Copy link
@pointlessone

pointlessone Dec 21, 2016

Author Contributor

Current is 69.7 MiB. The new one is 165.9 MiB. So yes, increase of almost 100 MiB.
What is the threshold?

This comment has been minimized.

Copy link
@gdiggs

gdiggs Dec 21, 2016

Contributor

Our spec says 512 MB so this should be ok!

@pointlessone

This comment has been minimized.

Copy link
Contributor Author

commented Dec 21, 2016

I personally try keeping my Rubocop config very slim and use community-driven default style.

But If I had to chose I'd probably enabled Bundler/DuplicatedGem, Performance/CompareWithBlock, Style/EmptyMethod, and Style/TernaryParentheses.

@maxjacobson

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

SGTM. That default config lives here. We can make that change separately.

@gdiggs gdiggs merged commit 1eec00b into codeclimate:master Dec 22, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate 18 fixed issues
Details
parser (>= 2.3.1.1, < 3.0)
powerpack (~> 0.1)
rainbow (>= 1.99.1, < 3.0)
ruby-progressbar (~> 1.7)
unicode-display_width (~> 1.0, >= 1.0.1)
rubocop-migrations (0.1.2)
rubocop (~> 0.41)
rubocop-rspec (1.5.0)
rubocop (>= 0.40.0)
rubocop-rspec (1.8.0)

This comment has been minimized.

Copy link
@andyw8

andyw8 Dec 22, 2016

Contributor

Thanks, I was just about to open a PR for this! (I'm one of the rubocop-rspec maintainers).

@pointlessone pointlessone deleted the pointlessone:dep-bump branch Dec 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.