JacobEvelyn / friends

Spend time with the people you care about. Introvert-tested. Extrovert-approved.

MIT License

872 stars 39 forks source link

Add rubocop dependency #240

Closed shen-sat closed 5 years ago

shen-sat commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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[bot] commented 5 years ago

Codecov Report

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

@@          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 commented 5 years ago

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 commented 5 years ago

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.