From c04f802101cb0ec31208267806a497fdfa2674f2 Mon Sep 17 00:00:00 2001 From: Greg Fletcher Date: Wed, 30 Jun 2021 08:27:52 -0400 Subject: [PATCH 1/2] [Fix #9890] Make the separator for the Style/CommentAnnotation cop configurable. By default, the enforced style will be the current behaviour of annotation keyword followed by a colon (`:`), and a space. A new supported style is added to only require a space following the annotation keyword. --- CHANGELOG.md | 1 + ...e_comment_annotation_style_configurable.md | 1 + config/default.yml | 4 + lib/rubocop/cop/style/comment_annotation.rb | 59 ++- .../cop/style/comment_annotation_spec.rb | 335 ++++++++++++------ 5 files changed, 291 insertions(+), 109 deletions(-) create mode 100644 changelog/fix_make_comment_annotation_style_configurable.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b73d5963bc..5b8b41c3dc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5722,3 +5722,4 @@ [@timlkelly]: https://github.com/timlkelly [@AirWick219]: https://github.com/AirWick219 [@markburns]: https://github.com/markburns +[@gregfletch]: https://github.com/gregfletch diff --git a/changelog/fix_make_comment_annotation_style_configurable.md b/changelog/fix_make_comment_annotation_style_configurable.md new file mode 100644 index 00000000000..64b4924488a --- /dev/null +++ b/changelog/fix_make_comment_annotation_style_configurable.md @@ -0,0 +1 @@ +* [#9890](https://github.com/rubocop/rubocop/issues/9890): Make colon after comment annotation configurable. ([@gregfletch][]) diff --git a/config/default.yml b/config/default.yml index 3cf7c7ea2ae..68bb3860b4c 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3150,6 +3150,10 @@ Style/CommentAnnotation: - HACK - REVIEW - NOTE + EnforcedStyle: colon + SupportedStyles: + - colon + - space Style/CommentedKeyword: Description: 'Do not place comments on the same line as certain keywords.' diff --git a/lib/rubocop/cop/style/comment_annotation.rb b/lib/rubocop/cop/style/comment_annotation.rb index fb62a145aa5..30e97812d6d 100644 --- a/lib/rubocop/cop/style/comment_annotation.rb +++ b/lib/rubocop/cop/style/comment_annotation.rb @@ -12,7 +12,7 @@ module Style # incorrect registering of keywords (eg. `review`) inside a paragraph as an # annotation. # - # @example + # @example EnforcedStyle: colon (default) # # bad # # TODO make better # @@ -36,14 +36,37 @@ module Style # # # good # # OPTIMIZE: does not work + # + # @example EnforcedStyle: space + # # bad + # # TODO: make better + # + # # good + # # TODO make better + # + # # bad + # # fixme does not work + # + # # good + # # FIXME does not work + # + # # bad + # # Optimize does not work + # + # # good + # # OPTIMIZE does not work class CommentAnnotation < Base include AnnotationComment + include ConfigurableEnforcedStyle include RangeHelp extend AutoCorrector - MSG = 'Annotation keywords like `%s` should be all ' \ - 'upper case, followed by a colon, and a space, ' \ - 'then a note describing the problem.' + MSG_COLON_STYLE = 'Annotation keywords like `%s` should be all ' \ + 'upper case, followed by a colon, and a space, ' \ + 'then a note describing the problem.' + MSG_SPACE_STYLE = 'Annotation keywords like `%s` should be all ' \ + 'upper case, followed by a space, ' \ + 'then a note describing the problem.' MISSING_NOTE = 'Annotation comment, with keyword `%s`, is missing a note.' def on_new_investigation @@ -63,13 +86,19 @@ def on_new_investigation private def register_offense(range, note, first_word) + message = if style == :colon + MSG_COLON_STYLE + else + MSG_SPACE_STYLE + end + add_offense( range, - message: format(note ? MSG : MISSING_NOTE, keyword: first_word) + message: format(note ? message : MISSING_NOTE, keyword: first_word) ) do |corrector| next if note.nil? - corrector.replace(range, "#{first_word.upcase}: ") + correct_offense(corrector, range, first_word) end end @@ -92,8 +121,26 @@ def concat_length(*args) end def correct_annotation?(first_word, colon, space, note) + return correct_colon_annotation?(first_word, colon, space, note) if style == :colon + + correct_space_annotation?(first_word, colon, space, note) + end + + def correct_colon_annotation?(first_word, colon, space, note) keyword?(first_word) && (colon && space && note || !colon && !note) end + + def correct_space_annotation?(first_word, colon, space, note) + keyword?(first_word) && (!colon && space && note || !colon && !note) + end + + def correct_offense(corrector, range, first_word) + if style == :colon + corrector.replace(range, "#{first_word.upcase}: ") + else + corrector.replace(range, "#{first_word.upcase} ") + end + end end end end diff --git a/spec/rubocop/cop/style/comment_annotation_spec.rb b/spec/rubocop/cop/style/comment_annotation_spec.rb index 43ae1794e84..e071b3db17a 100644 --- a/spec/rubocop/cop/style/comment_annotation_spec.rb +++ b/spec/rubocop/cop/style/comment_annotation_spec.rb @@ -1,144 +1,273 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::Style::CommentAnnotation, :config do - let(:cop_config) { { 'Keywords' => %w[TODO FIXME OPTIMIZE HACK REVIEW] } } + context 'with default EnforcedStyle configuration (colon + space)' do + let(:cop_config) { { 'Keywords' => %w[TODO FIXME OPTIMIZE HACK REVIEW] } } - context 'missing colon' do - it 'registers an offense and adds colon' do - expect_offense(<<~RUBY) - # TODO make better - ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. - RUBY + context 'missing colon' do + it 'registers an offense and adds colon' do + expect_offense(<<~RUBY) + # TODO make better + ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. + RUBY - expect_correction(<<~RUBY) - # TODO: make better - RUBY + expect_correction(<<~RUBY) + # TODO: make better + RUBY + end end - end - context 'with configured keyword' do - let(:cop_config) { { 'Keywords' => %w[ISSUE] } } + context 'with configured keyword' do + let(:cop_config) { { 'Keywords' => %w[ISSUE] } } - it 'registers an offense for a missing colon after the word' do - expect_offense(<<~RUBY) - # ISSUE wrong order - ^^^^^^ Annotation keywords like `ISSUE` should be all upper case, followed by a colon, and a space, then a note describing the problem. - RUBY + it 'registers an offense for a missing colon after the word' do + expect_offense(<<~RUBY) + # ISSUE wrong order + ^^^^^^ Annotation keywords like `ISSUE` should be all upper case, followed by a colon, and a space, then a note describing the problem. + RUBY - expect_correction(<<~RUBY) - # ISSUE: wrong order - RUBY + expect_correction(<<~RUBY) + # ISSUE: wrong order + RUBY + end end - end - context 'missing space after colon' do - it 'registers an offense and adds space' do - expect_offense(<<~RUBY) - # TODO:make better - ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. - RUBY + context 'missing space after colon' do + it 'registers an offense and adds space' do + expect_offense(<<~RUBY) + # TODO:make better + ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. + RUBY - expect_correction(<<~RUBY) - # TODO: make better - RUBY + expect_correction(<<~RUBY) + # TODO: make better + RUBY + end end - end - context 'lower case keyword' do - it 'registers an offense and upcases' do - expect_offense(<<~RUBY) - # fixme: does not work - ^^^^^^^ Annotation keywords like `fixme` should be all upper case, followed by a colon, and a space, then a note describing the problem. - RUBY + context 'lower case keyword' do + it 'registers an offense and upcases' do + expect_offense(<<~RUBY) + # fixme: does not work + ^^^^^^^ Annotation keywords like `fixme` should be all upper case, followed by a colon, and a space, then a note describing the problem. + RUBY - expect_correction(<<~RUBY) - # FIXME: does not work - RUBY + expect_correction(<<~RUBY) + # FIXME: does not work + RUBY + end end - end - context 'capitalized keyword' do - it 'registers an offense and upcases' do - expect_offense(<<~RUBY) - # Optimize: does not work - ^^^^^^^^^^ Annotation keywords like `Optimize` should be all upper case, followed by a colon, and a space, then a note describing the problem. - RUBY + context 'capitalized keyword' do + it 'registers an offense and upcases' do + expect_offense(<<~RUBY) + # Optimize: does not work + ^^^^^^^^^^ Annotation keywords like `Optimize` should be all upper case, followed by a colon, and a space, then a note describing the problem. + RUBY + + expect_correction(<<~RUBY) + # OPTIMIZE: does not work + RUBY + end + end - expect_correction(<<~RUBY) - # OPTIMIZE: does not work + context 'upper case keyword with colon by no note' do + it 'registers an offense without auto-correction' do + expect_offense(<<~RUBY) + # HACK: + ^^^^^ Annotation comment, with keyword `HACK`, is missing a note. + RUBY + + expect_no_corrections + end + end + + it 'accepts upper case keyword with colon, space and note' do + expect_no_offenses('# REVIEW: not sure about this') + end + + it 'accepts upper case keyword alone' do + expect_no_offenses('# OPTIMIZE') + end + + it 'accepts a comment that is obviously a code example' do + expect_no_offenses('# Todo.destroy(1)') + end + + it 'accepts a keyword that is just the beginning of a sentence' do + expect_no_offenses(<<~RUBY) + # Optimize if you want. I wouldn't recommend it. + # Hack is a fun game. RUBY end - end - context 'upper case keyword with colon by no note' do - it 'registers an offense without auto-correction' do - expect_offense(<<~RUBY) - # HACK: - ^^^^^ Annotation comment, with keyword `HACK`, is missing a note. + it 'accepts a keyword that is somewhere in a sentence' do + expect_no_offenses(<<~RUBY) + # Example: There are three reviews, with ranks 1, 2, and 3. A new + # review is saved with rank 2. The two reviews that originally had + # ranks 2 and 3 will have their ranks increased to 3 and 4. RUBY + end - expect_no_corrections + context 'when a keyword is not in the configuration' do + let(:cop_config) { { 'Keywords' => %w[FIXME OPTIMIZE HACK REVIEW] } } + + it 'accepts the word without colon' do + expect_no_offenses('# TODO make better') + end end - end - it 'accepts upper case keyword with colon, space and note' do - expect_no_offenses('# REVIEW: not sure about this') - end + context 'offenses in consecutive inline comments' do + it 'registers each of them' do + expect_offense(<<~RUBY) + class ToBeDone + ITEMS = [ + '', # TODO Item 1 + ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. + '', # TODO Item 2 + ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. + ].freeze + end + RUBY + end + end - it 'accepts upper case keyword alone' do - expect_no_offenses('# OPTIMIZE') + context 'multiline comment' do + it 'only registers an offense on the first line' do + expect_offense(<<~RUBY) + # TODO line 1 + ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. + # TODO line 2 + # TODO line 3 + RUBY + end + end end - it 'accepts a comment that is obviously a code example' do - expect_no_offenses('# Todo.destroy(1)') - end + context 'with space EnforcedStyle configuration' do + let(:cop_config) do + { + 'Keywords' => %w[TODO FIXME OPTIMIZE HACK REVIEW], + 'EnforcedStyle' => 'space' + } + end - it 'accepts a keyword that is just the beginning of a sentence' do - expect_no_offenses(<<~RUBY) - # Optimize if you want. I wouldn't recommend it. - # Hack is a fun game. - RUBY - end + context 'with colon' do + it 'registers an offense and removes colon' do + expect_offense(<<~RUBY) + # TODO: make better + ^^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a space, then a note describing the problem. + RUBY - it 'accepts a keyword that is somewhere in a sentence' do - expect_no_offenses(<<~RUBY) - # Example: There are three reviews, with ranks 1, 2, and 3. A new - # review is saved with rank 2. The two reviews that originally had - # ranks 2 and 3 will have their ranks increased to 3 and 4. - RUBY - end + expect_correction(<<~RUBY) + # TODO make better + RUBY + end + end + + context 'with configured keyword' do + let(:cop_config) { { 'Keywords' => %w[ISSUE], 'EnforcedStyle' => 'space' } } - context 'when a keyword is not in the configuration' do - let(:cop_config) { { 'Keywords' => %w[FIXME OPTIMIZE HACK REVIEW] } } + it 'registers an offense for containing a colon after the word' do + expect_offense(<<~RUBY) + # ISSUE: wrong order + ^^^^^^^ Annotation keywords like `ISSUE` should be all upper case, followed by a space, then a note describing the problem. + RUBY - it 'accepts the word without colon' do - expect_no_offenses('# TODO make better') + expect_correction(<<~RUBY) + # ISSUE wrong order + RUBY + end + end + + context 'lower case keyword' do + it 'registers an offense and upcases' do + expect_offense(<<~RUBY) + # fixme does not work + ^^^^^^ Annotation keywords like `fixme` should be all upper case, followed by a space, then a note describing the problem. + RUBY + + expect_correction(<<~RUBY) + # FIXME does not work + RUBY + end + end + + context 'upper case keyword with colon by no note' do + it 'registers an offense without auto-correction' do + expect_offense(<<~RUBY) + # HACK: + ^^^^^ Annotation comment, with keyword `HACK`, is missing a note. + RUBY + + expect_no_corrections + end end - end - context 'offenses in consecutive inline comments' do - it 'registers each of them' do - expect_offense(<<~RUBY) - class ToBeDone - ITEMS = [ - '', # TODO Item 1 - ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. - '', # TODO Item 2 - ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. - ].freeze - end + it 'accepts upper case keyword with colon, space and note' do + expect_no_offenses('# REVIEW not sure about this') + end + + it 'accepts upper case keyword alone' do + expect_no_offenses('# OPTIMIZE') + end + + it 'accepts a comment that is obviously a code example' do + expect_no_offenses('# Todo.destroy(1)') + end + + it 'accepts a keyword that is just the beginning of a sentence' do + expect_no_offenses(<<~RUBY) + # Optimize if you want. I wouldn't recommend it. + # Hack is a fun game. RUBY end - end - context 'multiline comment' do - it 'only registers an offense on the first line' do - expect_offense(<<~RUBY) - # TODO line 1 - ^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a colon, and a space, then a note describing the problem. - # TODO line 2 - # TODO line 3 + it 'accepts a keyword that is somewhere in a sentence' do + expect_no_offenses(<<~RUBY) + # Example: There are three reviews, with ranks 1, 2, and 3. A new + # review is saved with rank 2. The two reviews that originally had + # ranks 2 and 3 will have their ranks increased to 3 and 4. RUBY end + + context 'when a keyword is not in the configuration' do + let(:cop_config) do + { + 'Keywords' => %w[FIXME OPTIMIZE HACK REVIEW], + 'EnforcedStyle' => 'space' + } + end + + it 'accepts the word with colon' do + expect_no_offenses('# TODO: make better') + end + end + + context 'offenses in consecutive inline comments' do + it 'registers each of them' do + expect_offense(<<~RUBY) + class ToBeDone + ITEMS = [ + '', # TODO: Item 1 + ^^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a space, then a note describing the problem. + '', # TODO: Item 2 + ^^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a space, then a note describing the problem. + ].freeze + end + RUBY + end + end + + context 'multiline comment' do + it 'only registers an offense on the first line' do + expect_offense(<<~RUBY) + # TODO: line 1 + ^^^^^^ Annotation keywords like `TODO` should be all upper case, followed by a space, then a note describing the problem. + # TODO: line 2 + # TODO: line 3 + RUBY + end + end end end From 446843b0718d854c071d9838545c12f35a415459 Mon Sep 17 00:00:00 2001 From: Greg Fletcher Date: Wed, 30 Jun 2021 11:10:42 -0400 Subject: [PATCH 2/2] [Fix #9890] Address reviewer feedback and simplify configuration to a boolean flag indicating whether or not a colon separator is required. --- config/default.yml | 5 +--- lib/rubocop/cop/style/comment_annotation.rb | 25 ++++++++----------- .../cop/style/comment_annotation_spec.rb | 10 ++++---- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/config/default.yml b/config/default.yml index 68bb3860b4c..11af268c520 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3150,10 +3150,7 @@ Style/CommentAnnotation: - HACK - REVIEW - NOTE - EnforcedStyle: colon - SupportedStyles: - - colon - - space + RequireColon: true Style/CommentedKeyword: Description: 'Do not place comments on the same line as certain keywords.' diff --git a/lib/rubocop/cop/style/comment_annotation.rb b/lib/rubocop/cop/style/comment_annotation.rb index 30e97812d6d..24f7b092a0a 100644 --- a/lib/rubocop/cop/style/comment_annotation.rb +++ b/lib/rubocop/cop/style/comment_annotation.rb @@ -12,7 +12,7 @@ module Style # incorrect registering of keywords (eg. `review`) inside a paragraph as an # annotation. # - # @example EnforcedStyle: colon (default) + # @example RequireColon: true (default) # # bad # # TODO make better # @@ -37,7 +37,7 @@ module Style # # good # # OPTIMIZE: does not work # - # @example EnforcedStyle: space + # @example RequireColon: false # # bad # # TODO: make better # @@ -57,7 +57,6 @@ module Style # # OPTIMIZE does not work class CommentAnnotation < Base include AnnotationComment - include ConfigurableEnforcedStyle include RangeHelp extend AutoCorrector @@ -86,11 +85,7 @@ def on_new_investigation private def register_offense(range, note, first_word) - message = if style == :colon - MSG_COLON_STYLE - else - MSG_SPACE_STYLE - end + message = requires_colon? ? MSG_COLON_STYLE : MSG_SPACE_STYLE add_offense( range, @@ -121,7 +116,7 @@ def concat_length(*args) end def correct_annotation?(first_word, colon, space, note) - return correct_colon_annotation?(first_word, colon, space, note) if style == :colon + return correct_colon_annotation?(first_word, colon, space, note) if requires_colon? correct_space_annotation?(first_word, colon, space, note) end @@ -135,11 +130,13 @@ def correct_space_annotation?(first_word, colon, space, note) end def correct_offense(corrector, range, first_word) - if style == :colon - corrector.replace(range, "#{first_word.upcase}: ") - else - corrector.replace(range, "#{first_word.upcase} ") - end + return corrector.replace(range, "#{first_word.upcase}: ") if requires_colon? + + corrector.replace(range, "#{first_word.upcase} ") + end + + def requires_colon? + cop_config['RequireColon'] end end end diff --git a/spec/rubocop/cop/style/comment_annotation_spec.rb b/spec/rubocop/cop/style/comment_annotation_spec.rb index e071b3db17a..6dd3b57a464 100644 --- a/spec/rubocop/cop/style/comment_annotation_spec.rb +++ b/spec/rubocop/cop/style/comment_annotation_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::Style::CommentAnnotation, :config do - context 'with default EnforcedStyle configuration (colon + space)' do + context 'with default RequireColon configuration (colon + space)' do let(:cop_config) { { 'Keywords' => %w[TODO FIXME OPTIMIZE HACK REVIEW] } } context 'missing colon' do @@ -144,11 +144,11 @@ class ToBeDone end end - context 'with space EnforcedStyle configuration' do + context 'with RequireColon configuration set to false' do let(:cop_config) do { 'Keywords' => %w[TODO FIXME OPTIMIZE HACK REVIEW], - 'EnforcedStyle' => 'space' + 'RequireColon' => false } end @@ -166,7 +166,7 @@ class ToBeDone end context 'with configured keyword' do - let(:cop_config) { { 'Keywords' => %w[ISSUE], 'EnforcedStyle' => 'space' } } + let(:cop_config) { { 'Keywords' => %w[ISSUE], 'RequireColon' => false } } it 'registers an offense for containing a colon after the word' do expect_offense(<<~RUBY) @@ -235,7 +235,7 @@ class ToBeDone let(:cop_config) do { 'Keywords' => %w[FIXME OPTIMIZE HACK REVIEW], - 'EnforcedStyle' => 'space' + 'RequireColon' => false } end