Patch #31509
Add Rubocop to enforce some styles
Status: | Closed | Start date: | ||
---|---|---|---|---|
Priority: | Normal | Due date: | ||
Assignee: | % 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:
- 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
Associated revisions
Add RuboCop to enforce some styles (#31509).
Patch by Marius BALTEANU and Go MAEDA.
Update RuboCop to 0.72.0 (#31509).
Update RuboCop to 0.74.0 (#31509).
Add rubocop-performance (#31509).
Update RuboCop to 0.75.0 (#31509).
Update rubocop-performance to 1.5.0 (#31509).
Update RuboCop to 0.76.0 (#31509).
History
#1
Updated by Marius BALTEANU about 3 years 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 about 3 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.
#3
Updated by Marius BALTEANU about 3 years ago
- File deleted (
0001-Add-Rubocop-and-enable-the-following-cops.patch)
#4
Updated by Marius BALTEANU about 3 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 be2.3
instead of2.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 about 3 years ago
- File 31509-v2.diff
added
- Assignee set to Marius BALTEANU
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 about 3 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
#7
Updated by Marius BALTEANU about 3 years ago
- Assignee changed from Marius BALTEANU to Go MAEDA
#8
Updated by Kevin Fischer about 3 years ago
+1
#9
Updated by Go MAEDA about 3 years ago
- File 31509-v3.diff
added
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 about 3 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!
#11
Updated by Go MAEDA about 3 years ago
- Status changed from New to Closed
Committed. Thank you for your contribution.
#12
Updated by Go MAEDA about 3 years ago
- File rubocop-0_72_0.diff
added
- Status changed from Closed to Reopened
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 about 3 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.0Here is a patch to update RuboCop to 0.72.0.
Committed the patch in r18320.