From b667bcb48b26dab6c87827bd23ea99c6216bc7b4 Mon Sep 17 00:00:00 2001 From: Mike Angell <53470248+shopmike@users.noreply.github.com> Date: Fri, 20 Sep 2019 02:08:11 +1000 Subject: [PATCH] Shopify stye guide fixes (#1160) --- .rubocop_todo.yml | 48 +------------------------ lib/liquid/block_body.rb | 10 +++--- lib/liquid/lexer.rb | 20 +++++++---- lib/liquid/parse_context.rb | 1 - lib/liquid/standardfilters.rb | 2 +- lib/liquid/tags/for.rb | 2 +- lib/liquid/tags/if.rb | 4 +-- lib/liquid/tags/raw.rb | 2 +- lib/liquid/variable.rb | 2 +- performance/profile.rb | 2 +- performance/shopify/database.rb | 4 +-- test/integration/error_handling_test.rb | 10 ++++-- test/integration/template_test.rb | 20 ++++++++--- test/test_helper.rb | 2 +- test/unit/condition_unit_test.rb | 4 +-- test/unit/context_unit_test.rb | 20 ++++++++--- test/unit/tokenizer_unit_test.rb | 2 +- 17 files changed, 72 insertions(+), 83 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 127162899..34a2e2444 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -6,26 +6,6 @@ # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. -# Offense count: 2 -Lint/AmbiguousOperator: - Exclude: - - 'test/unit/condition_unit_test.rb' - -# Offense count: 21 -# Configuration parameters: AllowSafeAssignment. -Lint/AssignmentInCondition: - Exclude: - - 'lib/liquid/block_body.rb' - - 'lib/liquid/lexer.rb' - - 'lib/liquid/standardfilters.rb' - - 'lib/liquid/tags/for.rb' - - 'lib/liquid/tags/if.rb' - - 'lib/liquid/tags/raw.rb' - - 'lib/liquid/variable.rb' - - 'performance/profile.rb' - - 'test/test_helper.rb' - - 'test/unit/tokenizer_unit_test.rb' - # Offense count: 2 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyle. @@ -34,17 +14,6 @@ Lint/InheritException: Exclude: - 'lib/liquid/interrupts.rb' -# Offense count: 2 -Lint/UselessAssignment: - Exclude: - - 'performance/shopify/database.rb' - -# Offense count: 1 -# Configuration parameters: CheckForMethodsWithNoSideEffects. -Lint/Void: - Exclude: - - 'lib/liquid/parse_context.rb' - # Offense count: 98 # Cop supports --auto-correct. # Configuration parameters: AutoCorrect, AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. @@ -76,19 +45,4 @@ Style/ClassVars: Exclude: - 'lib/liquid/condition.rb' - 'lib/liquid/strainer.rb' - - 'lib/liquid/template.rb' - -# Offense count: 1 -# Configuration parameters: AllowCoercion. -Style/DateTime: - Exclude: - - 'test/unit/context_unit_test.rb' - -# Offense count: 9 -# Cop supports --auto-correct. -# Configuration parameters: AllowAsExpressionSeparator. -Style/Semicolon: - Exclude: - - 'test/integration/error_handling_test.rb' - - 'test/integration/template_test.rb' - - 'test/unit/context_unit_test.rb' + - 'lib/liquid/template.rb' \ No newline at end of file diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb index c4ce2671f..9400e383c 100644 --- a/lib/liquid/block_body.rb +++ b/lib/liquid/block_body.rb @@ -27,7 +27,7 @@ def parse(tokenizer, parse_context, &block) end private def parse_for_liquid_tag(tokenizer, parse_context) - while token = tokenizer.shift + while (token = tokenizer.shift) unless token.empty? || token =~ WhitespaceOrNothing unless token =~ LiquidTagToken # line isn't empty but didn't match tag syntax, yield and let the @@ -36,7 +36,7 @@ def parse(tokenizer, parse_context, &block) end tag_name = Regexp.last_match(1) markup = Regexp.last_match(2) - unless tag = registered_tags[tag_name] + unless (tag = registered_tags[tag_name]) # end parsing if we reach an unknown tag and let the caller decide # determine how to proceed return yield tag_name, markup @@ -52,7 +52,7 @@ def parse(tokenizer, parse_context, &block) end private def parse_for_document(tokenizer, parse_context, &block) - while token = tokenizer.shift + while (token = tokenizer.shift) next if token.empty? case when token.start_with?(TAGSTART) @@ -74,7 +74,7 @@ def parse(tokenizer, parse_context, &block) next parse_for_liquid_tag(liquid_tag_tokenizer, parse_context, &block) end - unless tag = registered_tags[tag_name] + unless (tag = registered_tags[tag_name]) # end parsing if we reach an unknown tag and let the caller decide # determine how to proceed return yield tag_name, markup @@ -122,7 +122,7 @@ def render_to_output_buffer(context, output) context.resource_limits.render_score += @nodelist.length idx = 0 - while node = @nodelist[idx] + while (node = @nodelist[idx]) previous_output_size = output.bytesize case node diff --git a/lib/liquid/lexer.rb b/lib/liquid/lexer.rb index 04e0c116a..a251c3e59 100644 --- a/lib/liquid/lexer.rb +++ b/lib/liquid/lexer.rb @@ -33,15 +33,21 @@ def tokenize until @ss.eos? @ss.skip(WHITESPACE_OR_NOTHING) break if @ss.eos? - tok = if t = @ss.scan(COMPARISON_OPERATOR) then [:comparison, t] - elsif t = @ss.scan(SINGLE_STRING_LITERAL) then [:string, t] - elsif t = @ss.scan(DOUBLE_STRING_LITERAL) then [:string, t] - elsif t = @ss.scan(NUMBER_LITERAL) then [:number, t] - elsif t = @ss.scan(IDENTIFIER) then [:id, t] - elsif t = @ss.scan(DOTDOT) then [:dotdot, t] + tok = if (t = @ss.scan(COMPARISON_OPERATOR)) + [:comparison, t] + elsif (t = @ss.scan(SINGLE_STRING_LITERAL)) + [:string, t] + elsif (t = @ss.scan(DOUBLE_STRING_LITERAL)) + [:string, t] + elsif (t = @ss.scan(NUMBER_LITERAL)) + [:number, t] + elsif (t = @ss.scan(IDENTIFIER)) + [:id, t] + elsif (t = @ss.scan(DOTDOT)) + [:dotdot, t] else c = @ss.getch - if s = SPECIALS[c] + if (s = SPECIALS[c]) [s, c] else raise SyntaxError, "Unexpected character #{c}" diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 2da3ad757..4afdbe596 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -21,7 +21,6 @@ def partial=(value) @partial = value @options = value ? partial_options : @template_options @error_mode = @options[:error_mode] || Template.error_mode - value end def partial_options diff --git a/lib/liquid/standardfilters.rb b/lib/liquid/standardfilters.rb index 517857acc..6855cd212 100644 --- a/lib/liquid/standardfilters.rb +++ b/lib/liquid/standardfilters.rb @@ -327,7 +327,7 @@ def newline_to_br(input) def date(input, format) return input if format.to_s.empty? - return input unless date = Utils.to_date(input) + return input unless (date = Utils.to_date(input)) date.strftime(format.to_s) end diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index d9613697f..5c7b56012 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -111,7 +111,7 @@ def strict_parse(markup) @reversed = p.id?('reversed') while p.look(:id) && p.look(:colon, 1) - unless attribute = p.id?('limit') || p.id?('offset') + unless (attribute = p.id?('limit') || p.id?('offset')) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_attribute") end p.consume diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index c3d1a777a..b68e309a6 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -94,7 +94,7 @@ def strict_parse(markup) def parse_binary_comparisons(p) condition = parse_comparison(p) first_condition = condition - while op = (p.id?('and') || p.id?('or')) + while (op = (p.id?('and') || p.id?('or'))) child_condition = parse_comparison(p) condition.send(op, child_condition) condition = child_condition @@ -104,7 +104,7 @@ def parse_binary_comparisons(p) def parse_comparison(p) a = Expression.parse(p.expression) - if op = p.consume?(:comparison) + if (op = p.consume?(:comparison)) b = Expression.parse(p.expression) Condition.new(a, op, b) else diff --git a/lib/liquid/tags/raw.rb b/lib/liquid/tags/raw.rb index fde3ee100..e4a78a817 100644 --- a/lib/liquid/tags/raw.rb +++ b/lib/liquid/tags/raw.rb @@ -13,7 +13,7 @@ def initialize(tag_name, markup, parse_context) def parse(tokens) @body = +'' - while token = tokens.shift + while (token = tokens.shift) if token =~ FullTokenPossiblyInvalid @body << Regexp.last_match(1) if Regexp.last_match(1) != "" return if block_delimiter == Regexp.last_match(2) diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 2fc2ea858..265748d3d 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -110,7 +110,7 @@ def parse_filter_expressions(filter_name, unparsed_args) filter_args = [] keyword_args = nil unparsed_args.each do |a| - if matches = a.match(JustTagAttributes) + if (matches = a.match(JustTagAttributes)) keyword_args ||= {} keyword_args[matches[1]] = Expression.parse(matches[2]) else diff --git a/performance/profile.rb b/performance/profile.rb index 101f6e5d6..70740778d 100644 --- a/performance/profile.rb +++ b/performance/profile.rb @@ -15,7 +15,7 @@ end end - if profile_type == :cpu && graph_filename = ENV['GRAPH_FILENAME'] + if profile_type == :cpu && (graph_filename = ENV['GRAPH_FILENAME']) File.open(graph_filename, 'w') do |f| StackProf::Report.new(results).print_graphviz(nil, f) end diff --git a/performance/shopify/database.rb b/performance/shopify/database.rb index 9836cd464..2db6d300c 100644 --- a/performance/shopify/database.rb +++ b/performance/shopify/database.rb @@ -32,8 +32,8 @@ def self.tables db['article'] = db['blog']['articles'].first db['cart'] = { - 'total_price' => db['line_items'].values.inject(0) { |sum, item| sum += item['line_price'] * item['quantity'] }, - 'item_count' => db['line_items'].values.inject(0) { |sum, item| sum += item['quantity'] }, + 'total_price' => db['line_items'].values.inject(0) { |sum, item| sum + item['line_price'] * item['quantity'] }, + 'item_count' => db['line_items'].values.inject(0) { |sum, item| sum + item['quantity'] }, 'items' => db['line_items'].values, } diff --git a/test/integration/error_handling_test.rb b/test/integration/error_handling_test.rb index 265632cb3..7abaec040 100644 --- a/test/integration/error_handling_test.rb +++ b/test/integration/error_handling_test.rb @@ -211,7 +211,10 @@ def test_default_exception_renderer_with_internal_error def test_setting_default_exception_renderer old_exception_renderer = Liquid::Template.default_exception_renderer exceptions = [] - Liquid::Template.default_exception_renderer = ->(e) { exceptions << e; '' } + Liquid::Template.default_exception_renderer = ->(e) { + exceptions << e + '' + } template = Liquid::Template.parse('This is a runtime error: {{ errors.argument_error }}') output = template.render('errors' => ErrorDrop.new) @@ -225,7 +228,10 @@ def test_setting_default_exception_renderer def test_exception_renderer_exposing_non_liquid_error template = Liquid::Template.parse('This is a runtime error: {{ errors.runtime_error }}', line_numbers: true) exceptions = [] - handler = ->(e) { exceptions << e; e.cause } + handler = ->(e) { + exceptions << e + e.cause + } output = template.render({ 'errors' => ErrorDrop.new }, exception_renderer: handler) diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index 75dd95b4e..48549f55a 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -81,7 +81,10 @@ def test_persistent_assigns_squash_instance_assigns def test_lambda_is_called_once_from_persistent_assigns_over_multiple_parses_and_renders t = Template.new - t.assigns['number'] = -> { @global ||= 0; @global += 1 } + t.assigns['number'] = -> { + @global ||= 0 + @global += 1 + } assert_equal '1', t.parse("{{number}}").render! assert_equal '1', t.parse("{{number}}").render! assert_equal '1', t.render! @@ -90,7 +93,10 @@ def test_lambda_is_called_once_from_persistent_assigns_over_multiple_parses_and_ def test_lambda_is_called_once_from_custom_assigns_over_multiple_parses_and_renders t = Template.new - assigns = { 'number' => -> { @global ||= 0; @global += 1 } } + assigns = { 'number' => -> { + @global ||= 0 + @global += 1 + } } assert_equal '1', t.parse("{{number}}").render!(assigns) assert_equal '1', t.parse("{{number}}").render!(assigns) assert_equal '1', t.render!(assigns) @@ -237,7 +243,10 @@ def test_render_bang_force_rethrow_errors_on_passed_context def test_exception_renderer_that_returns_string exception = nil - handler = ->(e) { exception = e; '' } + handler = ->(e) { + exception = e + '' + } output = Template.parse("{{ 1 | divided_by: 0 }}").render({}, exception_renderer: handler) @@ -248,7 +257,10 @@ def test_exception_renderer_that_returns_string def test_exception_renderer_that_raises exception = nil assert_raises(Liquid::ZeroDivisionError) do - Template.parse("{{ 1 | divided_by: 0 }}").render({}, exception_renderer: ->(e) { exception = e; raise }) + Template.parse("{{ 1 | divided_by: 0 }}").render({}, exception_renderer: ->(e) { + exception = e + raise + }) end assert exception.is_a?(Liquid::ZeroDivisionError) end diff --git a/test/test_helper.rb b/test/test_helper.rb index d7a664138..9606ef8fb 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -9,7 +9,7 @@ require 'liquid/profiler' mode = :strict -if env_mode = ENV['LIQUID_PARSER_MODE'] +if (env_mode = ENV['LIQUID_PARSER_MODE']) puts "-- #{env_mode.upcase} ERROR MODE" mode = env_mode.to_sym end diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index 69f6b902b..8d4e02f9d 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -26,9 +26,9 @@ def test_default_operators_evalute_true assert_evaluates_true 1, '<=', 1 # negative numbers assert_evaluates_true 1, '>', -1 - assert_evaluates_true -1, '<', 1 + assert_evaluates_true(-1, '<', 1) assert_evaluates_true 1.0, '>', -1.0 - assert_evaluates_true -1.0, '<', 1.0 + assert_evaluates_true(-1.0, '<', 1.0) end def test_default_operators_evalute_false diff --git a/test/unit/context_unit_test.rb b/test/unit/context_unit_test.rb index fe790cfb0..3b460d7a3 100644 --- a/test/unit/context_unit_test.rb +++ b/test/unit/context_unit_test.rb @@ -85,7 +85,7 @@ def test_variables @context['date'] = Date.today assert_equal Date.today, @context['date'] - now = DateTime.now + now = Time.now @context['datetime'] = now assert_equal now, @context['datetime'] @@ -405,7 +405,11 @@ def test_array_containing_lambda_as_variable end def test_lambda_is_called_once - @context['callcount'] = proc { @global ||= 0; @global += 1; @global.to_s } + @context['callcount'] = proc { + @global ||= 0 + @global += 1 + @global.to_s + } assert_equal '1', @context['callcount'] assert_equal '1', @context['callcount'] @@ -415,7 +419,11 @@ def test_lambda_is_called_once end def test_nested_lambda_is_called_once - @context['callcount'] = { "lambda" => proc { @global ||= 0; @global += 1; @global.to_s } } + @context['callcount'] = { "lambda" => proc { + @global ||= 0 + @global += 1 + @global.to_s + } } assert_equal '1', @context['callcount.lambda'] assert_equal '1', @context['callcount.lambda'] @@ -425,7 +433,11 @@ def test_nested_lambda_is_called_once end def test_lambda_in_array_is_called_once - @context['callcount'] = [1, 2, proc { @global ||= 0; @global += 1; @global.to_s }, 4, 5] + @context['callcount'] = [1, 2, proc { + @global ||= 0 + @global += 1 + @global.to_s + }, 4, 5] assert_equal '1', @context['callcount[2]'] assert_equal '1', @context['callcount[2]'] diff --git a/test/unit/tokenizer_unit_test.rb b/test/unit/tokenizer_unit_test.rb index d094aa1f7..44342d6c1 100644 --- a/test/unit/tokenizer_unit_test.rb +++ b/test/unit/tokenizer_unit_test.rb @@ -35,7 +35,7 @@ def test_calculate_line_numbers_per_token_with_profiling def tokenize(source) tokenizer = Liquid::Tokenizer.new(source) tokens = [] - while t = tokenizer.shift + while (t = tokenizer.shift) tokens << t end tokens