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

Add ForceEqualSignAlignment to .rubocop.yml #1190

Merged
merged 6 commits into from Oct 21, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Expand Up @@ -892,7 +892,7 @@ Lint/FormatParameterMismatch:
Enabled: true

Lint/HandleExceptions:
Enabled: true
AllowComments: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea to edit this file since it is auto-generated using upstream data.
If this change is absolutely necessary for this PR to pass, the better alternative would be to redefine the setting in the local .rubocop.yml

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one comes down from a change to the Shopify style guide. I've been pulling this out of recent PR's

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.. Then you (@shopmike) should push this change to master directly then. It may not be ideal but a lot better than pulling the change out from multiple PRs..

Shopify could also consider shipping the Official Style Guide as a Ruby gem so that its updates do not explicitly show up in downstream repos.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the plan is to push this through as its own PR, just hasn't been a priority.

The config rarely changes and when it does I don't mind the fact it forces everyone to follow it as generally it would only be done when enough people have agreed that it applies to all projects.


Lint/ImplicitStringConcatenation:
Description: Checks for adjacent string literals on the same line, which could
Expand Down
6 changes: 6 additions & 0 deletions .rubocop.yml
Expand Up @@ -15,3 +15,9 @@ AllCops:
Naming/MethodName:
Exclude:
- 'example/server/liquid_servlet.rb'

Layout/ExtraSpacing:
ForceEqualSignAlignment: true

Layout/SpaceAroundOperators:
Enabled: false
2 changes: 1 addition & 1 deletion Rakefile
Expand Up @@ -11,7 +11,7 @@ desc('run test suite with default parser')
Rake::TestTask.new(:base_test) do |t|
t.libs << '.' << 'lib' << 'test'
t.test_files = FileList['test/{integration,unit}/**/*_test.rb']
t.verbose = false
t.verbose = false
end

desc('run test suite with warn error mode')
Expand Down
8 changes: 4 additions & 4 deletions example/server/liquid_servlet.rb
Expand Up @@ -12,16 +12,16 @@ def do_POST(req, res)
private

def handle(_type, req, res)
@request = req
@request = req
@response = res

@request.path_info =~ /(\w+)\z/
@action = Regexp.last_match(1) || 'index'
@action = Regexp.last_match(1) || 'index'
@assigns = send(@action) if respond_to?(@action)

@response['Content-Type'] = "text/html"
@response.status = 200
@response.body = Liquid::Template.parse(read_template).render(@assigns, filters: [ProductsFilter])
@response.status = 200
@response.body = Liquid::Template.parse(read_template).render(@assigns, filters: [ProductsFilter])
end

def read_template(filename = @action)
Expand Down
30 changes: 15 additions & 15 deletions lib/liquid/block_body.rb
Expand Up @@ -2,18 +2,18 @@

module Liquid
class BlockBody
LiquidTagToken = /\A\s*(\w+)\s*(.*?)\z/o
FullToken = /\A#{TagStart}#{WhitespaceControl}?(\s*)(\w+)(\s*)(.*?)#{WhitespaceControl}?#{TagEnd}\z/om
ContentOfVariable = /\A#{VariableStart}#{WhitespaceControl}?(.*?)#{WhitespaceControl}?#{VariableEnd}\z/om
LiquidTagToken = /\A\s*(\w+)\s*(.*?)\z/o
FullToken = /\A#{TagStart}#{WhitespaceControl}?(\s*)(\w+)(\s*)(.*?)#{WhitespaceControl}?#{TagEnd}\z/om
ContentOfVariable = /\A#{VariableStart}#{WhitespaceControl}?(.*?)#{WhitespaceControl}?#{VariableEnd}\z/om
WhitespaceOrNothing = /\A\s*\z/
TAGSTART = "{%"
VARSTART = "{{"
TAGSTART = "{%"
VARSTART = "{{"

attr_reader :nodelist

def initialize
@nodelist = []
@blank = true
@blank = true
end

def parse(tokenizer, parse_context, &block)
Expand All @@ -34,15 +34,15 @@ def parse(tokenizer, parse_context, &block)
# caller raise a syntax error
return yield token, token
end
tag_name = Regexp.last_match(1)
markup = Regexp.last_match(2)
tag_name = Regexp.last_match(1)
markup = Regexp.last_match(2)
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
end
new_tag = tag.parse(tag_name, markup, tokenizer, parse_context)
@blank &&= new_tag.blank?
new_tag = tag.parse(tag_name, markup, tokenizer, parse_context)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does RuboCop force this realignment if you were to revert manually and then autocorrect after?
The reason I ask is the benefits of operator-alignment is lost when there is a block of code separating two realigned lines..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only thing that breaks the behaviour is an empty line from when I was playing around with this. In cases where the formatting isn't ideal, you need to space it.

@blank &&= new_tag.blank?
@nodelist << new_tag
end
parse_context.line_number = tokenizer.line_number
Expand All @@ -61,7 +61,7 @@ def parse(tokenizer, parse_context, &block)
raise_missing_tag_terminator(token, parse_context)
end
tag_name = Regexp.last_match(2)
markup = Regexp.last_match(4)
markup = Regexp.last_match(4)

if parse_context.line_number
# newlines inside the tag should increase the line number,
Expand All @@ -79,8 +79,8 @@ def parse(tokenizer, parse_context, &block)
# determine how to proceed
return yield tag_name, markup
end
new_tag = tag.parse(tag_name, markup, tokenizer, parse_context)
@blank &&= new_tag.blank?
new_tag = tag.parse(tag_name, markup, tokenizer, parse_context)
@blank &&= new_tag.blank?
@nodelist << new_tag
when token.start_with?(VARSTART)
whitespace_handler(token, parse_context)
Expand All @@ -92,7 +92,7 @@ def parse(tokenizer, parse_context, &block)
end
parse_context.trim_whitespace = false
@nodelist << token
@blank &&= !!(token =~ WhitespaceOrNothing)
@blank &&= !!(token =~ WhitespaceOrNothing)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the side-effect of using automated corrections.. 😞 :
As a human, you can tell from a distance that this change significantly hampers readability instead of improving it..
(View the entire file for proper judgement).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good way to be able to find where needing to space should be done. Searching for 15+ spaces provides most of the weird auto-corrections in which case I would space

end
parse_context.line_number = tokenizer.line_number
end
Expand Down Expand Up @@ -121,7 +121,7 @@ def render(context)
def render_to_output_buffer(context, output)
context.resource_limits.render_score += @nodelist.length

idx = 0
idx = 0
while (node = @nodelist[idx])
previous_output_size = output.bytesize

Expand Down
16 changes: 8 additions & 8 deletions lib/liquid/condition.rb
Expand Up @@ -35,16 +35,16 @@ def self.operators
attr_accessor :left, :operator, :right

def initialize(left = nil, operator = nil, right = nil)
@left = left
@operator = operator
@right = right
@child_relation = nil
@left = left
@operator = operator
@right = right
@child_relation = nil
@child_condition = nil
end

def evaluate(context = Context.new)
condition = self
result = nil
result = nil
loop do
result = interpret_condition(condition.left, condition.right, condition.operator, context)

Expand All @@ -62,12 +62,12 @@ def evaluate(context = Context.new)
end

def or(condition)
@child_relation = :or
@child_relation = :or
@child_condition = condition
end

def and(condition)
@child_relation = :and
@child_relation = :and
@child_condition = condition
end

Expand Down Expand Up @@ -115,7 +115,7 @@ def interpret_condition(left, right, op, context)
# return this as the result.
return context.evaluate(left) if op.nil?

left = context.evaluate(left)
left = context.evaluate(left)
right = context.evaluate(right)

operation = self.class.operators[op] || raise(Liquid::ArgumentError, "Unknown operator #{op}")
Expand Down
22 changes: 11 additions & 11 deletions lib/liquid/context.rb
Expand Up @@ -41,8 +41,8 @@ def initialize(environments = {}, outer_scope = {}, registers = {}, rethrow_erro
self.exception_renderer = ->(_e) { raise }
end

@interrupts = []
@filters = []
@interrupts = []
@filters = []
@global_filter = nil
end
# rubocop:enable Metrics/ParameterLists
Expand All @@ -60,7 +60,7 @@ def strainer
# Note that this does not register the filters with the main Template object. see <tt>Template.register_filter</tt>
# for that
def add_filters(filters)
filters = [filters].flatten.compact
filters = [filters].flatten.compact
@filters += filters
@strainer = nil
end
Expand All @@ -85,9 +85,9 @@ def pop_interrupt
end

def handle_error(e, line_number = nil)
e = internal_error unless e.is_a?(Liquid::Error)
e = internal_error unless e.is_a?(Liquid::Error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if you could replace single letter variables with something longer yet apt to mitigate this excess whitespace..

If its not too much trouble, I recommend that you follow this suggestion for all similar scenarios

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only issue with most of these examples is that by making the variable longer, the next line is usually the same variable but accessing long method names or nested methods.

Copy link
Contributor Author

@alebruck alebruck Oct 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we keep this cop and try to address these cases individually by small refactoring to improve readability or should we postpone the enforcement of this cop?

e.template_name ||= template_name
e.line_number ||= line_number
e.line_number ||= line_number
errors.push(e)
exception_renderer.call(e).to_s
end
Expand Down Expand Up @@ -138,12 +138,12 @@ def new_isolated_subcontext
static_environments: static_environments,
registers: StaticRegisters.new(registers)
).tap do |subcontext|
subcontext.base_scope_depth = base_scope_depth + 1
subcontext.base_scope_depth = base_scope_depth + 1
subcontext.exception_renderer = exception_renderer
subcontext.filters = @filters
subcontext.strainer = nil
subcontext.errors = errors
subcontext.warnings = warnings
subcontext.filters = @filters
subcontext.strainer = nil
subcontext.errors = errors
subcontext.warnings = warnings
end
end

Expand Down Expand Up @@ -188,7 +188,7 @@ def find_variable(key, raise_on_not_found: true)
try_variable_find_in_environments(key, raise_on_not_found: raise_on_not_found)
end

variable = variable.to_liquid
variable = variable.to_liquid
variable.context = self if variable.respond_to?(:context=)

variable
Expand Down
26 changes: 13 additions & 13 deletions lib/liquid/errors.rb
Expand Up @@ -40,19 +40,19 @@ def message_prefix
end
end

ArgumentError = Class.new(Error)
ContextError = Class.new(Error)
FileSystemError = Class.new(Error)
StandardError = Class.new(Error)
SyntaxError = Class.new(Error)
StackLevelError = Class.new(Error)
TaintedError = Class.new(Error)
MemoryError = Class.new(Error)
ZeroDivisionError = Class.new(Error)
FloatDomainError = Class.new(Error)
UndefinedVariable = Class.new(Error)
ArgumentError = Class.new(Error)
ContextError = Class.new(Error)
FileSystemError = Class.new(Error)
StandardError = Class.new(Error)
SyntaxError = Class.new(Error)
StackLevelError = Class.new(Error)
TaintedError = Class.new(Error)
MemoryError = Class.new(Error)
ZeroDivisionError = Class.new(Error)
FloatDomainError = Class.new(Error)
UndefinedVariable = Class.new(Error)
UndefinedDropMethod = Class.new(Error)
UndefinedFilter = Class.new(Error)
UndefinedFilter = Class.new(Error)
MethodOverrideError = Class.new(Error)
InternalError = Class.new(Error)
InternalError = Class.new(Error)
end
2 changes: 1 addition & 1 deletion lib/liquid/expression.rb
Expand Up @@ -7,7 +7,7 @@ class MethodLiteral

def initialize(method_name, to_s)
@method_name = method_name
@to_s = to_s
@to_s = to_s
end

def to_liquid
Expand Down
2 changes: 1 addition & 1 deletion lib/liquid/file_system.rb
Expand Up @@ -47,7 +47,7 @@ class LocalFileSystem
attr_accessor :root

def initialize(root, pattern = "_%s.liquid")
@root = root
@root = root
@pattern = pattern
end

Expand Down
6 changes: 3 additions & 3 deletions lib/liquid/forloop_drop.rb
Expand Up @@ -3,10 +3,10 @@
module Liquid
class ForloopDrop < Drop
def initialize(name, length, parentloop)
@name = name
@length = length
@name = name
@length = length
@parentloop = parentloop
@index = 0
@index = 0
end

attr_reader :name, :length, :parentloop
Expand Down
14 changes: 7 additions & 7 deletions lib/liquid/lexer.rb
Expand Up @@ -3,7 +3,7 @@
require "strscan"
module Liquid
class Lexer
SPECIALS = {
SPECIALS = {
'|' => :pipe,
'.' => :dot,
':' => :colon,
Expand All @@ -15,12 +15,12 @@ class Lexer
'?' => :question,
'-' => :dash,
}.freeze
IDENTIFIER = /[a-zA-Z_][\w-]*\??/
IDENTIFIER = /[a-zA-Z_][\w-]*\??/
SINGLE_STRING_LITERAL = /'[^\']*'/
DOUBLE_STRING_LITERAL = /"[^\"]*"/
NUMBER_LITERAL = /-?\d+(\.\d+)?/
DOTDOT = /\.\./
COMPARISON_OPERATOR = /==|!=|<>|<=?|>=?|contains(?=\s)/
NUMBER_LITERAL = /-?\d+(\.\d+)?/
DOTDOT = /\.\./
COMPARISON_OPERATOR = /==|!=|<>|<=?|>=?|contains(?=\s)/
WHITESPACE_OR_NOTHING = /\s*/

def initialize(input)
Expand All @@ -33,7 +33,7 @@ def tokenize
until @ss.eos?
@ss.skip(WHITESPACE_OR_NOTHING)
break if @ss.eos?
tok = if (t = @ss.scan(COMPARISON_OPERATOR))
tok = if (t = @ss.scan(COMPARISON_OPERATOR))
[:comparison, t]
elsif (t = @ss.scan(SINGLE_STRING_LITERAL))
[:string, t]
Expand All @@ -46,7 +46,7 @@ def tokenize
elsif (t = @ss.scan(DOTDOT))
[:dotdot, t]
else
c = @ss.getch
c = @ss.getch
if (s = SPECIALS[c])
[s, c]
else
Expand Down
12 changes: 6 additions & 6 deletions lib/liquid/parse_context.rb
Expand Up @@ -7,19 +7,19 @@ class ParseContext

def initialize(options = {})
@template_options = options ? options.dup : {}
@locale = @template_options[:locale] ||= I18n.new
@warnings = []
self.depth = 0
self.partial = false
@locale = @template_options[:locale] ||= I18n.new
@warnings = []
self.depth = 0
self.partial = false
end

def [](option_key)
@options[option_key]
end

def partial=(value)
@partial = value
@options = value ? partial_options : @template_options
@partial = value
@options = value ? partial_options : @template_options
@error_mode = @options[:error_mode] || Template.error_mode
end

Expand Down
8 changes: 4 additions & 4 deletions lib/liquid/parse_tree_visitor.rb
Expand Up @@ -11,14 +11,14 @@ def self.for(node, callbacks = Hash.new(proc {}))
end

def initialize(node, callbacks)
@node = node
@node = node
@callbacks = callbacks
end

def add_callback_for(*classes, &block)
callback = block
callback = ->(node, _) { yield node } if block.arity.abs == 1
callback = ->(_, _) { yield } if block.arity.zero?
callback = block
callback = ->(node, _) { yield node } if block.arity.abs == 1
callback = ->(_, _) { yield } if block.arity.zero?
classes.each { |klass| @callbacks[klass] = callback }
self
end
Expand Down