Patch #31509

Add Rubocop to enforce some styles

Added by Marius BALTEANU 5 months ago. Updated 4 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Code cleanup/refactoring
Target version:4.1.0

Description

I propose to add Rubocop to enforce some styles for those who contribute with patches to Redmine or for those who review the proposed patches.

For the moment, I propose to start only with 2 cops:

  1. Layout/TrailingWhitespace:
    To not have anymore the annoying trailing whitespaces. I used this cop the prepare the patches from #31506 and #31507.
  1. Style/FrozenStringLiteralComment:
    To ensure that the new ruby files added to the codebase have the frozen string literal. As I've mentioned in #31508, we already missed some files.

To run the checks: bundle exec rubocop

0001-Add-Rubocop-and-enable-the-following-cops.patch Magnifier (1.59 KB) Marius BALTEANU, 2019-06-06 10:16

31509-v2.diff Magnifier (68.6 KB) Go MAEDA, 2019-06-08 09:56

31509-v3.diff Magnifier (69.1 KB) Go MAEDA, 2019-06-18 17:05

rubocop-0_72_0.diff Magnifier (2.59 KB) Go MAEDA, 2019-06-26 07:25

Associated revisions

Revision 18259
Added by Go MAEDA 4 months ago

Add RuboCop to enforce some styles (#31509).

Patch by Marius BALTEANU and Go MAEDA.

Revision 18320
Added by Go MAEDA 4 months ago

Update RuboCop to 0.72.0 (#31509).

Revision 18371
Added by Go MAEDA 2 months ago

Update RuboCop to 0.74.0 (#31509).

Revision 18378
Added by Go MAEDA 2 months ago

Add rubocop-performance (#31509).

Revision 18576
Added by Go MAEDA 19 days ago

Update RuboCop to 0.75.0 (#31509).

Revision 18579
Added by Go MAEDA 18 days ago

Update rubocop-performance to 1.5.0 (#31509).

History

#1 Updated by Marius BALTEANU 5 months ago

  • Target version set to 4.1.0

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

#2 Updated by Go MAEDA 5 months ago

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.

#3 Updated by Marius BALTEANU 5 months ago

  • File deleted (0001-Add-Rubocop-and-enable-the-following-cops.patch)

#4 Updated by Marius BALTEANU 5 months ago

Go MAEDA wrote:

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.

Great, thanks for your quick reply. Here is the updated patch.

#5 Updated by Go MAEDA 4 months ago

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

With the original patch, RuboCop performs only a few checks. But I want to perform various checks with RuboCop when I write or review patches.

#6 Updated by Marius BALTEANU 4 months ago

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

#7 Updated by Marius BALTEANU 4 months ago

  • Assignee changed from Marius BALTEANU to Go MAEDA

#8 Updated by Kevin Fischer 4 months ago

+1

#9 Updated by Go MAEDA 4 months ago

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

#10 Updated by Marius BALTEANU 4 months ago

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!

#11 Updated by Go MAEDA 4 months ago

  • Status changed from New to Closed

Committed. Thank you for your contribution.

#12 Updated by Go MAEDA 4 months ago

RuboCop 0.72.0 has been released on July 25th.
https://rubygems.org/gems/rubocop/versions/0.72.0

Here is a patch to update RuboCop to 0.72.0.

#13 Updated by Go MAEDA 4 months ago

  • Status changed from Reopened to Closed

Go MAEDA wrote:

RuboCop 0.72.0 has been released on July 25th.
https://rubygems.org/gems/rubocop/versions/0.72.0

Here is a patch to update RuboCop to 0.72.0.

Committed the patch in r18320.

Also available in: Atom PDF