Redmine

Marius BALTEANU wrote:

Go Maeda, do you see any downside for having Rubocop as part of the core?

I am in favor of adding RuboCop. Actually, I sometimes check patches with RuboCop on my dev environment.

But I think TargetRubyVersion in the suggested patch should be 2.3 instead of 2.5 because the oldest Ruby version Redmine supports is 2.3.

Go MAEDA wrote:

I propose adding .rubocop_todo.yml and removing "DisabledByDefault: true".

I like your proposal, but I think we should apply the following changes on top of your patch:

vagrant@jessie:/vagrant/project/redmine$ git diff
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 16a8d62..1cdb965 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -491,6 +491,7 @@ Layout/SpaceAroundOperators:
 # SupportedStylesForEmptyBraces: space, no_space
 Layout/SpaceBeforeBlockBraces:
   EnforcedStyleForEmptyBraces: no_space
+  Enabled: false

 # Offense count: 7
 # Cop supports --auto-correct.
@@ -1302,6 +1303,12 @@ Rails/TimeZone:
 Rails/Validation:
   Enabled: false

+# Offense count: 3
+Rails/BulkChangeTable:
+  Exclude:
+    - 'db/migrate/20120714122200_add_workflows_rule_fields.rb'
+    - 'db/migrate/20131214094309_remove_custom_fields_min_max_length_default_values.rb'
+
 # Offense count: 4
 Security/Eval:
   Exclude:

Otherwise, we'll have the following fails: https://gitlab.com/redmine-org/redmine/-/jobs/229555011

+1

Go MAEDA wrote:

I have updated the patch:

• Added rules that Marius pointed out in #31509#note-6. I have added rules to .rubocop.yml instead of updating .rubocop_todo.yml because .rubocop_todo.yml is an auto-generated file and it will be overwritten in the future
• Removed Layout/TrailingWhitespace. Since all checks are enabled now, we don't have to explicitly enable it
• Added Style/HashSyntax to allow Ruby 1.8 style hash syntax ({:foo => 1, :bar => 2})
• Updated doc/RUNNING_TESTS

Looks very good to me, thank you for improving my patch!