From 066430502b9141c5c7ed8c26fb8ce6e0bf87755c Mon Sep 17 00:00:00 2001 From: Eric Delaney Date: Tue, 17 Oct 2017 13:23:20 -0700 Subject: [PATCH] (PUP-8038) Update DecorateString to look for sentences Updated DecorateString cop to look for strings that match a regular expression for a basic sentence. This greatly reduces the number of strings that the cop expects to be decorated. 'A sentence that should be decorated.' --- CHANGELOG.md | 1 + README.md | 37 +++++++++- .../cop/i18n/gettext/decorate_string.rb | 56 +++++++++++++--- .../cop/i18n/gettext/decorate_string_spec.rb | 67 ++++++++++++++++--- 4 files changed, 139 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9933524..8cbc0db 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 118c540..b01ab48 100644 --- a/README.md +++ b/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) ``` diff --git a/lib/rubocop/cop/i18n/gettext/decorate_string.rb b/lib/rubocop/cop/i18n/gettext/decorate_string.rb index d9c661c..4779137 100644 --- a/lib/rubocop/cop/i18n/gettext/decorate_string.rb +++ b/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 diff --git a/spec/rubocop/cop/i18n/gettext/decorate_string_spec.rb b/spec/rubocop/cop/i18n/gettext/decorate_string_spec.rb index 3225614..d3eb0de 100644 --- a/spec/rubocop/cop/i18n/gettext/decorate_string_spec.rb +++ b/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