Patch #31509
closed
Add Rubocop to enforce some styles
Added by Marius BĂLTEANU over 5 years ago.
Updated over 5 years ago.
Category:
Code cleanup/refactoring
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:
- Layout/TrailingWhitespace:
To not have anymore the annoying trailing whitespaces. I used this cop the prepare the patches from #31506 and #31507.
- 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
- Target version set to 4.1.0
Go Maeda, do you see any downside for having Rubocop as part of the core?
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.
- File deleted (
0001-Add-Rubocop-and-enable-the-following-cops.patch)
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.
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.
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
- Assignee changed from Marius BĂLTEANU to Go MAEDA
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
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!
- Status changed from New to Closed
Committed. Thank you for your contribution.
- Status changed from Reopened to Closed
Also available in: Atom
PDF
Updated by 
Updated by 
Updated by