Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update rubocop-shopify (2.7.0 -> 2.12.0) #1687

Merged
merged 6 commits into from Feb 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions .rubocop.yml
Expand Up @@ -12,13 +12,18 @@ Performance:
AllCops:
TargetRubyVersion: 2.7
NewCops: disable
SuggestExtensions: false
Exclude:
- 'vendor/bundle/**/*'

Naming/MethodName:
Exclude:
- 'example/server/liquid_servlet.rb'

# Backport https://github.com/Shopify/ruby-style-guide/pull/258
Layout/BeginEndAlignment:
Enabled: true
Style/ClassMethodsDefinitions:
Enabled: false

# liquid filter calls were being mistaken to be calls on arrays
Style/ConcatArrayLiterals:
karreiro marked this conversation as resolved.
Show resolved Hide resolved
Exclude:
- 'test/integration/standard_filter_test.rb'
2 changes: 1 addition & 1 deletion Gemfile
Expand Up @@ -19,7 +19,7 @@ end

group :test do
gem 'rubocop', '~> 1.44.0'
gem 'rubocop-shopify', '~> 2.7.0', require: false
gem 'rubocop-shopify', '~> 2.12.0', require: false
gem 'rubocop-performance', require: false

platform :mri, :truffleruby do
Expand Down
14 changes: 9 additions & 5 deletions example/server/example_servlet.rb
Expand Up @@ -30,14 +30,18 @@ def products
private

def products_list
[{ 'name' => 'Arbor Draft', 'price' => 39900, 'description' => 'the *arbor draft* is a excellent product' },
{ 'name' => 'Arbor Element', 'price' => 40000, 'description' => 'the *arbor element* rocks for freestyling' },
{ 'name' => 'Arbor Diamond', 'price' => 59900, 'description' => 'the *arbor diamond* is a made up product because im obsessed with arbor and have no creativity' }]
[
{ 'name' => 'Arbor Draft', 'price' => 39900, 'description' => 'the *arbor draft* is a excellent product' },
{ 'name' => 'Arbor Element', 'price' => 40000, 'description' => 'the *arbor element* rocks for freestyling' },
{ 'name' => 'Arbor Diamond', 'price' => 59900, 'description' => 'the *arbor diamond* is a made up product because im obsessed with arbor and have no creativity' }
]
end

def more_products_list
[{ 'name' => 'Arbor Catalyst', 'price' => 39900, 'description' => 'the *arbor catalyst* is an advanced drop-through for freestyle and flatground performance and versatility' },
{ 'name' => 'Arbor Fish', 'price' => 40000, 'description' => 'the *arbor fish* is a compact pin that features an extended wheelbase and time-honored teardrop shape' }]
[
{ 'name' => 'Arbor Catalyst', 'price' => 39900, 'description' => 'the *arbor catalyst* is an advanced drop-through for freestyle and flatground performance and versatility' },
{ 'name' => 'Arbor Fish', 'price' => 40000, 'description' => 'the *arbor fish* is a compact pin that features an extended wheelbase and time-honored teardrop shape' }
]
end

def description
Expand Down
12 changes: 8 additions & 4 deletions lib/liquid/block.rb
Expand Up @@ -36,13 +36,17 @@ def unknown_tag(tag_name, _markup, _tokenizer)
# @api private
def self.raise_unknown_tag(tag, block_name, block_delimiter, parse_context)
if tag == 'else'
raise SyntaxError, parse_context.locale.t("errors.syntax.unexpected_else",
block_name: block_name)
raise SyntaxError, parse_context.locale.t(
"errors.syntax.unexpected_else",
block_name: block_name,
)
elsif tag.start_with?('end')
raise SyntaxError, parse_context.locale.t("errors.syntax.invalid_delimiter",
raise SyntaxError, parse_context.locale.t(
"errors.syntax.invalid_delimiter",
tag: tag,
block_name: block_name,
block_delimiter: block_delimiter)
block_delimiter: block_delimiter,
)
else
raise SyntaxError, parse_context.locale.t("errors.syntax.unknown_tag", tag: tag)
end
Expand Down
6 changes: 4 additions & 2 deletions lib/liquid/condition.rb
Expand Up @@ -159,8 +159,10 @@ def deprecated_default_context
class ParseTreeVisitor < Liquid::ParseTreeVisitor
def children
[
@node.left, @node.right,
@node.child_condition, @node.attachment
@node.left,
@node.right,
@node.child_condition,
@node.attachment
].compact
end
end
Expand Down
2 changes: 1 addition & 1 deletion lib/liquid/context.rb
Expand Up @@ -144,7 +144,7 @@ def new_isolated_subcontext
self.class.build(
resource_limits: resource_limits,
static_environments: static_environments,
registers: Registers.new(registers)
registers: Registers.new(registers),
).tap do |subcontext|
subcontext.base_scope_depth = base_scope_depth + 1
subcontext.exception_renderer = exception_renderer
Expand Down
5 changes: 4 additions & 1 deletion lib/liquid/expression.rb
Expand Up @@ -3,7 +3,10 @@
module Liquid
class Expression
LITERALS = {
nil => nil, 'nil' => nil, 'null' => nil, '' => nil,
nil => nil,
'nil' => nil,
'null' => nil,
'' => nil,
'true' => true,
'false' => false,
'blank' => '',
Expand Down
2 changes: 1 addition & 1 deletion lib/liquid/standardfilters.rb
Expand Up @@ -25,7 +25,7 @@ module StandardFilters
STRIP_HTML_BLOCKS = Regexp.union(
%r{<script.*?</script>}m,
/<!--.*?-->/m,
%r{<style.*?</style>}m
%r{<style.*?</style>}m,
)
STRIP_HTML_TAGS = /<.*?>/m

Expand Down
2 changes: 1 addition & 1 deletion lib/liquid/tags/case.rb
Expand Up @@ -77,7 +77,7 @@ def render_to_output_buffer(context, output)
end

result = Liquid::Utils.to_liquid_value(
block.evaluate(context)
block.evaluate(context),
)

if result
Expand Down
2 changes: 1 addition & 1 deletion lib/liquid/tags/if.rb
Expand Up @@ -53,7 +53,7 @@ def unknown_tag(tag, markup, tokens)
def render_to_output_buffer(context, output)
@blocks.each do |block|
result = Liquid::Utils.to_liquid_value(
block.evaluate(context)
block.evaluate(context),
)

if result
Expand Down
2 changes: 1 addition & 1 deletion lib/liquid/tags/include.rb
Expand Up @@ -57,7 +57,7 @@ def render_to_output_buffer(context, output)
partial = PartialCache.load(
template_name,
context: context,
parse_context: parse_context
parse_context: parse_context,
)

context_variable_name = @alias_name || template_name.split('/').last
Expand Down
2 changes: 1 addition & 1 deletion lib/liquid/tags/render.rb
Expand Up @@ -69,7 +69,7 @@ def render_tag(context, output)
partial = PartialCache.load(
template_name,
context: context,
parse_context: parse_context
parse_context: parse_context,
)

context_variable_name = @alias_name || template_name.split('/').last
Expand Down
4 changes: 2 additions & 2 deletions lib/liquid/tags/unless.rb
Expand Up @@ -23,7 +23,7 @@ def render_to_output_buffer(context, output)
# First condition is interpreted backwards ( if not )
first_block = @blocks.first
result = Liquid::Utils.to_liquid_value(
first_block.evaluate(context)
first_block.evaluate(context),
)

unless result
Expand All @@ -33,7 +33,7 @@ def render_to_output_buffer(context, output)
# After the first condition unless works just like if
@blocks[1..-1].each do |block|
result = Liquid::Utils.to_liquid_value(
block.evaluate(context)
block.evaluate(context),
)

if result
Expand Down
4 changes: 2 additions & 2 deletions performance/shopify/paginate.rb
Expand Up @@ -45,8 +45,8 @@ def render_to_output_buffer(context, output)

pagination['items'] = collection_size
pagination['pages'] = page_count - 1
pagination['previous'] = link('&laquo; Previous', current_page - 1) unless 1 >= current_page
pagination['next'] = link('Next &raquo;', current_page + 1) unless page_count <= current_page + 1
pagination['previous'] = link('&laquo; Previous', current_page - 1) if 1 < current_page
pagination['next'] = link('Next &raquo;', current_page + 1) if page_count > current_page + 1
pagination['parts'] = []

hellip_break = false
Expand Down
25 changes: 17 additions & 8 deletions test/integration/assign_test.rb
Expand Up @@ -14,28 +14,37 @@ def test_assign_with_hyphen_in_variable_name
end

def test_assigned_variable
assert_template_result('.foo.',
assert_template_result(
'.foo.',
'{% assign foo = values %}.{{ foo[0] }}.',
{ 'values' => %w(foo bar baz) })
{ 'values' => %w(foo bar baz) },
)

assert_template_result('.bar.',
assert_template_result(
'.bar.',
'{% assign foo = values %}.{{ foo[1] }}.',
{ 'values' => %w(foo bar baz) })
{ 'values' => %w(foo bar baz) },
)
end

def test_assign_with_filter
assert_template_result('.bar.',
assert_template_result(
'.bar.',
'{% assign foo = values | split: "," %}.{{ foo[1] }}.',
{ 'values' => "foo,bar,baz" })
{ 'values' => "foo,bar,baz" },
)
end

def test_assign_syntax_error
assert_match_syntax_error(/assign/, '{% assign foo not values %}.')
end

def test_assign_uses_error_mode
assert_match_syntax_error("Expected dotdot but found pipe in ",
"{% assign foo = ('X' | downcase) %}", error_mode: :strict)
assert_match_syntax_error(
"Expected dotdot but found pipe in ",
"{% assign foo = ('X' | downcase) %}",
error_mode: :strict,
)
assert_template_result("", "{% assign foo = ('X' | downcase) %}", error_mode: :lax)
end

Expand Down
27 changes: 19 additions & 8 deletions test/integration/blank_test.rb
Expand Up @@ -57,9 +57,11 @@ def test_captures_are_blank

def test_nested_blocks_are_blank_but_only_if_all_children_are
assert_template_result("", wrap(wrap(" ")))
assert_template_result("\n but this is not " * (N + 1),
assert_template_result(
"\n but this is not " * (N + 1),
wrap('{% if true %} {% comment %} this is blank {% endcomment %} {% endif %}
{% if true %} but this is not {% endif %}'))
{% if true %} but this is not {% endif %}'),
)
end

def test_assigns_are_blank
Expand Down Expand Up @@ -89,12 +91,21 @@ def test_raw_is_not_blank
end

def test_include_is_blank
assert_template_result("foobar" * (N + 1), wrap("{% include 'foobar' %}"),
partials: { 'foobar' => 'foobar' })
assert_template_result(" foobar " * (N + 1), wrap("{% include ' foobar ' %}"),
partials: { ' foobar ' => ' foobar ' })
assert_template_result(" " * (N + 1), wrap(" {% include ' ' %} "),
partials: { ' ' => ' ' })
assert_template_result(
"foobar" * (N + 1),
wrap("{% include 'foobar' %}"),
partials: { 'foobar' => 'foobar' },
)
assert_template_result(
" foobar " * (N + 1),
wrap("{% include ' foobar ' %}"),
partials: { ' foobar ' => ' foobar ' },
)
assert_template_result(
" " * (N + 1),
wrap(" {% include ' ' %} "),
partials: { ' ' => ' ' },
)
end

def test_case_is_blank
Expand Down
56 changes: 35 additions & 21 deletions test/integration/context_test.rb
Expand Up @@ -121,14 +121,23 @@ def test_scoping
end

def test_length_query
assert_template_result("true", "{% if numbers.size == 4 %}true{% endif %}",
{ "numbers" => [1, 2, 3, 4] })
assert_template_result(
"true",
"{% if numbers.size == 4 %}true{% endif %}",
{ "numbers" => [1, 2, 3, 4] },
)

assert_template_result("true", "{% if numbers.size == 4 %}true{% endif %}",
{ "numbers" => { 1 => 1, 2 => 2, 3 => 3, 4 => 4 } })
assert_template_result(
"true",
"{% if numbers.size == 4 %}true{% endif %}",
{ "numbers" => { 1 => 1, 2 => 2, 3 => 3, 4 => 4 } },
)

assert_template_result("true", "{% if numbers.size == 1000 %}true{% endif %}",
{ "numbers" => { 1 => 1, 2 => 2, 3 => 3, 4 => 4, 'size' => 1000 } })
assert_template_result(
"true",
"{% if numbers.size == 1000 %}true{% endif %}",
{ "numbers" => { 1 => 1, 2 => 2, 3 => 3, 4 => 4, 'size' => 1000 } },
)
end

def test_hyphenated_variable
Expand Down Expand Up @@ -228,12 +237,14 @@ def test_recoursive_array_notation
end

def test_hash_to_array_transition
assigns = { 'colors' => {
'Blue' => ['003366', '336699', '6699CC', '99CCFF'],
'Green' => ['003300', '336633', '669966', '99CC99'],
'Yellow' => ['CC9900', 'FFCC00', 'FFFF99', 'FFFFCC'],
'Red' => ['660000', '993333', 'CC6666', 'FF9999'],
} }
assigns = {
'colors' => {
'Blue' => ['003366', '336699', '6699CC', '99CCFF'],
'Green' => ['003300', '336633', '669966', '99CC99'],
'Yellow' => ['CC9900', 'FFCC00', 'FFFF99', 'FFFFCC'],
'Red' => ['660000', '993333', 'CC6666', 'FF9999'],
},
}

assert_template_result("003366", "{{ colors.Blue[0] }}", assigns)
assert_template_result("FF9999", "{{ colors.Red[3] }}", assigns)
Expand Down Expand Up @@ -410,10 +421,12 @@ def test_lambda_is_called_once
def test_nested_lambda_is_called_once
@global = 0

@context['callcount'] = { "lambda" => proc {
@global += 1
@global.to_s
} }
@context['callcount'] = {
"lambda" => proc {
@global += 1
@global.to_s
},
}

assert_equal('1', @context['callcount.lambda'])
assert_equal('1', @context['callcount.lambda'])
Expand All @@ -423,10 +436,11 @@ def test_nested_lambda_is_called_once
def test_lambda_in_array_is_called_once
@global = 0

@context['callcount'] = [1, 2, proc {
@global += 1
@global.to_s
}, 4, 5]
p = proc {
@global += 1
@global.to_s
}
@context['callcount'] = [1, 2, p, 4, 5]

assert_equal('1', @context['callcount[2]'])
assert_equal('1', @context['callcount[2]'])
Expand Down Expand Up @@ -473,7 +487,7 @@ def test_apply_global_filter
def test_static_environments_are_read_with_lower_priority_than_environments
context = Context.build(
static_environments: { 'shadowed' => 'static', 'unshadowed' => 'static' },
environments: { 'shadowed' => 'dynamic' }
environments: { 'shadowed' => 'dynamic' },
)

assert_equal('dynamic', context['shadowed'])
Expand Down