Merge pull request #25 from ericldelaney/task/master/PUP-8038/fix_dec…

…orate_string_cop_for_sentences

(PUP-8038) Update DecorateString to look for sentences

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## master (unreleased)
 
+ * Updated DecorateString to look for sentences using a regular expression that should be decorated. This limits the number of strings that it finds to things that look like a sentence.
  * Code restructure (no API changes)
  * RuboCop lint fixes
 

README.md

@@ -26,7 +26,7 @@ require:
  - rubocop-i18n
 ...
 GetText/DecorateString:
-  Enabled: false
+  Enabled: true
 GetText/DecorateFunctionMessage:
   Enabled: true
 GetText/DecorateStringFormattingUsingInterpolation
@@ -37,6 +37,36 @@ GetText/DecorateStringFormattingUsingPercent
 
 ## Cops
 
+### GetText/DecorateString
+
+This cop is looks for strings that appear to be sentences but are not decorated. 
+Sentences are determined by the STRING_REGEXP.
+
+##### Error message thrown
+
+```
+decorator is missing around sentence
+```
+
+##### Bad
+
+``` ruby
+"Result is bad."
+```
+
+##### Good
+
+``` ruby
+_("Result is good.")
+```
+
+##### Ignored
+``` ruby
+"string"
+"A string with out a punctuation at the end"
+"a string that doesn't start with a capital letter."
+```
+
 ### GetText/DecorateFunctionMessage
 
 This cop looks for any raise or fail functions and checks that the user visible message is using gettext decoration with the _() function.
@@ -207,8 +237,9 @@ raise(_("Warning is %{value}") % { value: 'bad' })
 
 It may be necessary to ignore a cop for a particular piece of code. We follow standard rubocop idioms.
 ``` ruby
-raise("We don't want this translated")  # rubocop:disable GetText/DecorateFunctionMessage
-raise(_("We don't want this translated #{crazy}")  # rubocop:disable GetText/DecorateStringFormattingUsingInterpolation)
+raise("We don't want this translated.")                 # rubocop:disable GetText/DecorateString
+raise("We don't want this translated")                  # rubocop:disable GetText/DecorateFunctionMessage
+raise(_("We don't want this translated #{crazy}")       # rubocop:disable GetText/DecorateStringFormattingUsingInterpolation)
 raise(_("We don't want this translated %s") % ['crazy'] # rubocop:disable GetText/DecorateStringFormattingUsingPercent)
 ```
 

lib/rubocop/cop/i18n/gettext/decorate_string.rb

@@ -4,33 +4,69 @@ module RuboCop
   module Cop
     module I18n
       module GetText
-        # This cop checks for butt or ass,
-        # which is redundant.
+        # This cop is looks for strings that appear to be sentences but are not decorated.
+        # Sentences are determined by the STRING_REGEXP. (Upper case character, at least one space,
+        # and sentence punctuation at the end)
         #
         # @example
         #
         #   # bad
         #
-        #   "result is #{something.to_s}"
+        #   "Result is bad."
         #
         # @example
         #
         #   # good
         #
-        #   "result is #{something}"
+        #   _("Result is good.")
         class DecorateString < Cop
+          STRING_REGEXP = /^\s*[[:upper:]][[:alpha:]]*[[:blank:]]+.*[.!?]$/
+
+          def on_dstr(node)
+            check_for_parent_decorator(node) if dstr_contains_sentence?(node)
+          end
+
           def on_str(node)
-            str = node.children[0]
-            # ignore strings with no whitespace - are typically keywords or interpolation statements and cover the above commented-out statements
-            if str !~ /^\S*$/
-              add_offense(node, :expression, 'decorator is missing around sentence') if node.loc.respond_to?(:begin)
+            return unless sentence?(node)
+
+            parent = node.parent
+            if parent.respond_to?(:type)
+              return if parent.type == :regexp || parent.type == :dstr
             end
+
+            check_for_parent_decorator(node)
           end
 
           private
 
-          def message(node)
-            node.receiver ? MSG_DEFAULT : MSG_SELF
+          def sentence?(node)
+            child = node.children[0]
+            if child.is_a?(String)
+              if child.valid_encoding?
+                child.encode(Encoding::UTF_8).chomp =~ STRING_REGEXP
+              else
+                false
+              end
+            elsif child.respond_to?(:type) && child.type == :str
+              sentence?(child)
+            else
+              false
+            end
+          end
+
+          def dstr_contains_sentence?(node)
+            node.children.any? { |child| sentence?(child) }
+          end
+
+          def check_for_parent_decorator(node)
+            parent = node.parent
+            if parent.respond_to?(:type) && parent.type == :send
+              method_name = parent.loc.selector.source
+              return if GetText.supported_decorator?(method_name)
+            elsif parent.respond_to?(:method_name) && parent.method_name == :[]
+              return
+            end
+            add_offense(node, :expression, 'decorator is missing around sentence')
           end
         end
       end

spec/rubocop/cop/i18n/gettext/decorate_string_spec.rb

@@ -5,18 +5,67 @@
 describe RuboCop::Cop::I18n::GetText::DecorateString do
   let(:config) { RuboCop::Config.new }
   subject(:cop) { described_class.new(config) }
+  before(:each) do
+    investigate(cop, source)
+  end
+
+  context 'decoration needed for string' do
+    it_behaves_like 'a_detecting_cop', 'a = "A sentence that is not decorated."', '_', 'decorator is missing around sentence'
+    it_behaves_like 'a_detecting_cop', 'thing("A sentence that is not decorated.")', '_', 'decorator is missing around sentence'
+  end
+
+  context 'decoration not needed for string' do
+    it_behaves_like 'a_no_cop_required', "'keyword'"
+    # a regexp is a string
+    it_behaves_like 'a_no_cop_required', '@regexp = /[ =]/'
+    it_behaves_like 'a_no_cop_required', 'A sentence with out punctuation at the end'
+    it_behaves_like 'a_no_cop_required', 'Dir[File.dirname(__FILE__) + "/parser/compiler/catalog_validator/*.rb"].each { |f| require f }'
+    it_behaves_like 'a_no_cop_required', 'stream.puts "#@version\n" if @version'
+    it_behaves_like 'a_no_cop_required', '
+          f.puts(<<-YAML)
+            ---
+            :tag: yaml
+            :yaml:
+               :something: #{variable}
+          YAML'
+  end
+
+  context 'decoration not needed for a hash key' do
+    # a string as an hash key is ok
+    it_behaves_like 'a_no_cop_required', 'memo["#{class_path}##{method}-#{source_line}"] += 1'
+  end
+
+  context 'string with invalid UTF-8' do
+    it_behaves_like 'a_no_cop_required', '
+        STRING_MAP = {
+      Encoding::UTF_8 => "\uFFFD",
+      Encoding::UTF_16LE => "\xFD\xFF".force_encoding(Encoding::UTF_16LE),
+    }'
+  end
+
+  context 'decoration missing for dstr' do
+    it_behaves_like 'a_detecting_cop', "b = \"A sentence line one.
+        line two\"", '_', 'decorator is missing around sentence'
+    it_behaves_like 'a_detecting_cop', "b = \"line one.
+        A sentence line two.\"", '_', 'decorator is missing around sentence'
+  end
 
-  context 'undecorated string' do
-    let(:source) do
-      <<-RUBY
-        "a string"
-      RUBY
+  RuboCop::Cop::I18n::GetText.supported_decorators.each do |decorator|
+    context "#{decorator} already present" do
+      it_behaves_like 'a_no_cop_required', "#{decorator}('a string')"
+      it_behaves_like 'a_no_cop_required', "#{decorator} \"a string\""
+      it_behaves_like 'a_no_cop_required', "a = #{decorator}('a string')"
+      it_behaves_like 'a_no_cop_required', "#{decorator}(\"a %-5.2.s thing s string\")"
+      it_behaves_like 'a_no_cop_required', "Log.warning #{decorator}(\"could not change to group %{group}: %{detail}\") % { group: group, detail: detail }"
+      it_behaves_like 'a_no_cop_required', "Log.warning #{decorator}(\"could not change to group %{group}: %{detail}\") %
+                                            { group: #{decorator}(\"group\"), detail: #{decorator}(\"detail\") }"
     end
 
-    it 'rejects' do
-      investigate(cop, source)
-      expect(cop.offenses.size).to eq(1)
-      expect(cop.offenses[0].message).to match(/decorator is missing around sentence/)
+    context "#{decorator} around dstr" do
+      it_behaves_like 'a_no_cop_required', "a = #{decorator}(\"A sentence line one.
+        line two\")"
+      it_behaves_like 'a_no_cop_required', "a = #{decorator}(\"line one.
+        A sentence line two.\")"
     end
   end
 end