Update RuboCop configuration to inherit from theforeman-rubocop #886
Conversation
There was a problem hiding this comment.
Hi Archana!
I'm sorry it took me so long to review this PR, I wanted to triple check everything in the config file since this will end up being pretty important for devs and QE. I had a few questions and suggestions there but all in all everything looks great.
I expanded on this below but I don't think we can call .update on an ActiveRecord::Relation. It's not in any documentation I could find. I asked ChatGPT if the changes were ok and it said NO 3 times and YES once 🤷♂️. I'd put everything in a .each loop if validations are important and would keep the lines as .update_all if not.
db/migrate/20210720000001_remove_old_insights_statuses.foreman_rh_cloud.rb
Outdated
Show resolved
Hide resolved
db/migrate/20221102110254_fix_rh_cloud_settings_category_to_dsl.rb
Outdated
Show resolved
Hide resolved
archanaserver
force-pushed
the
rubocop
branch
2 times, most recently
from
5c1b15f to
4279ba9
Compare
Yes you are right, I had similar conversation few weeks back in one of my another PR on update and update_all, I misunderstood few things here in the code, but I was following the RuboCop doc: https://docs.rubocop.org/rubocop-rails/cops_rails.html#railsskipsmodelvalidations So I did this to few other places as well that is disabling this cop because it is not safe to use. |
- Added theforeman-rubocop as a development dependency in the gemspec. - Changed the RuboCop configuration to inherit from default.yml provided by theforeman-rubocop for better style alignment. - Removed redundant cops as they are already included in the inherited default.yml or not needed anymore.
There was a problem hiding this comment.
The changes look great to me, thank you! I'm totally ok with keeping update_all instead of refactoring into an each loop; I can see several ways that could end up a big headache. I just had one question about the included styles (posted elsewhere) but I'm happy to ACK once that is resolved.
There was a problem hiding this comment.
Thanks @archanaserver for your contribution and @qcjames53 for reviewing this, looks good from my end too.
|
Thanks @chris1984 and @qcjames53 :) |
by theforeman-rubocop for better style alignment.
default.yml or not needed anymore.
default.yml: https://github.com/theforeman/theforeman-rubocop/?tab=readme-ov-file#basic-style---default-performance-and-rails-cops
This is part of Rubocop standerdization, link for the reference: https://community.theforeman.org/t/standardizing-rubocop-with-theforeman-rubocop/37239