-
Notifications
You must be signed in to change notification settings - Fork 0
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 #48
Add rubocop #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for doing this; it'll help keep things clean moving forward. One thing I noticed in the rubocop output is this message:
Tip: Based on detected gems, the following RuboCop extension libraries might be helpful:
- rubocop-rails (https://rubygems.org/gems/rubocop-rails)
- rubocop-rspec (https://rubygems.org/gems/rubocop-rspec)
I think it would be a good idea to add these extensions in if possible. They'll give us more linting coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things. I have one comment below 👇 . Also, I think we need to add this:
AllCops:
NewCops: enable
to the rubocop.yml. I think this should get around the error. It also might fix the issue below.
.circleci/config.yml
Outdated
- run: | ||
name: "Rubocop-Rspec" | ||
command: | | ||
docker-compose exec web bundle exec rubocop -R --only RSpec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to split these up. Just running rubocop should cover the others. It's been a while since I've set up rubocop, though, so i may be wrong. If they're not running together, then we should find a way to get them to run together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is running with Rubocop. This command would give the option to only run on Rubocop for Rspec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that just running bundle exec rubocop
should run all the cops (basic rubocop, rails, and rspec). We don't want to split it up into 3 different instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They run together. This is just an additional option to only run rubocop for rspec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then we should remove the rubocop-rails and rubocop-rspec CI "run" instances and just do the regular rubocop one. When we meet again we should go over how CI works and how these configs work to run certain commands.
e0599f1
to
948db75
Compare
Fixes #30