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

Add rubocop dependency #240

Closed
wants to merge 8 commits into from

Conversation

shen-sat
Copy link
Contributor

@shen-sat shen-sat commented Sep 29, 2019


Hi there! Thanks so much for submitting a pull request!

Let's just make sure together that all of these boxes are checked before we
merge this change:

  • The code in these changes works correctly.
  • Code has tests as appropriate.
  • Code has been reviewed by @JacobEvelyn.
  • All tests pass on Travis CI.
  • Code coverage remains high.
  • Rubocop reports no issues on Travis CI.
  • The README.md file is updated as appropriate.

Don't worry—this list will get checked off in no time!

@shen-sat
Copy link
Contributor Author

Hi @JacobEvelyn, here is the change to add rubocop to the project - I've also added a dependabot config file that should prevent automatic pull requests for rubocop updates. If you still get the pull requests, let me know and I'll look into it 👍🏾

@JacobEvelyn
Copy link
Owner

Oof—so it looks like this is probably the reason why I didn't already have this in the gemspec. That version of Dependabot requires Ruby 2.2+, which breaks the build on our 2.1 tests.

I guess the question now is whether it's better to officially drop support for Ruby 2.1, or stick with the existing system as-is. On the one hand, it would be nice to include this dependency in the gemspec file. On the other, I don't love the idea of dropping support for a Ruby version when we don't need to. Thoughts?

@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #240 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #240   +/-   ##
======================================
  Coverage    98.3%   98.3%           
======================================
  Files          23      23           
  Lines         709     709           
======================================
  Hits          697     697           
  Misses         12      12

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0a44f4...3037f80. Read the comment docs.

@shen-sat
Copy link
Contributor Author

shen-sat commented Oct 1, 2019

Hey @JacobEvelyn, I've managed to find a workaround - custom Gemfiles ie a Gemfile for local development (and local use of rubocop 🙌🏾) and a Gemfile.ci for Travis (which only uses rubocop for ruby v2.6).

This does now mean the project has two Gemfiles (perhaps the Gemfile.ci could be moved into its own directory?).

I realise I've been pushing for rubocop locally quite a bit when I already have an issue (ie set home) which I should be focusing on (apologies if I've spent too long on rubocop!). If you feel like two Gemfiles might be overkill, feel free to close this pull request without merging. I've learnt so much re: dependabot and travis that I won't consider it a negative outcome and I will be free to focus my efforts on building the set home feature for Friends :)

@JacobEvelyn
Copy link
Owner

This is mostly superseded by #239, with the exception of the Dependabot config file. I'm not sure Dependabot will even try to bump the things that are in the Gemfile when there's a gemspec, but I guess we'll see.

@shen-sat shen-sat closed this Oct 3, 2019
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