Project

General

Profile

Actions

Patch #31509

closed

Add Rubocop to enforce some styles

Added by Marius BĂLTEANU almost 5 years ago. Updated almost 5 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Code cleanup/refactoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:

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


Files

0001-Add-Rubocop-and-enable-the-following-cops.patch (1.59 KB) 0001-Add-Rubocop-and-enable-the-following-cops.patch Marius BĂLTEANU, 2019-06-06 10:16
31509-v2.diff (68.6 KB) 31509-v2.diff Go MAEDA, 2019-06-08 09:56
31509-v3.diff (69.1 KB) 31509-v3.diff Go MAEDA, 2019-06-18 17:05
rubocop-0_72_0.diff (2.59 KB) rubocop-0_72_0.diff Go MAEDA, 2019-06-26 07:25
Actions #1

Updated by Marius BĂLTEANU almost 5 years ago

  • Target version set to 4.1.0

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

Actions #2

Updated by Go MAEDA almost 5 years 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.

Actions #3

Updated by Marius BĂLTEANU almost 5 years ago

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

Updated by Marius BĂLTEANU almost 5 years 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.

Actions #5

Updated by Go MAEDA almost 5 years 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.

Actions #6

Updated by Marius BĂLTEANU almost 5 years 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

Actions #7

Updated by Marius BĂLTEANU almost 5 years ago

  • Assignee changed from Marius BĂLTEANU to Go MAEDA
Actions #8

Updated by Kevin Fischer almost 5 years ago

+1

Actions #9

Updated by Go MAEDA almost 5 years 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
Actions #10

Updated by Marius BĂLTEANU almost 5 years 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!

Actions #11

Updated by Go MAEDA almost 5 years ago

  • Status changed from New to Closed

Committed. Thank you for your contribution.

Actions #12

Updated by Go MAEDA almost 5 years 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.

Actions #13

Updated by Go MAEDA almost 5 years 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.

Actions

Also available in: Atom PDF