From e19a4b8be9ccbf47e33815c617355602553b48ac Mon Sep 17 00:00:00 2001 From: Sam Rawlins Date: Tue, 18 Aug 2015 21:26:29 -0700 Subject: [PATCH] Rewrite SpaceBetweenParens linter to reduce false positives --- lib/scss_lint/linter/space_between_parens.rb | 116 +++++++++-- lib/scss_lint/sass/script.rb | 19 ++ .../linter/space_between_parens_spec.rb | 196 +++++++++++++++++- 3 files changed, 310 insertions(+), 21 deletions(-) diff --git a/lib/scss_lint/linter/space_between_parens.rb b/lib/scss_lint/linter/space_between_parens.rb index 59275afe..3a471732 100644 --- a/lib/scss_lint/linter/space_between_parens.rb +++ b/lib/scss_lint/linter/space_between_parens.rb @@ -1,36 +1,112 @@ module SCSSLint + # rubocop:disable Metrics/AbcSize + # Checks for the presence of spaces between parentheses. class Linter::SpaceBetweenParens < Linter include LinterRegistry - def visit_root(_node) + def check_node(node) + check(node, source_from_range(node.source_range)) + yield + end + + alias_method :visit_atroot, :check_node + alias_method :visit_cssimport, :check_node + alias_method :visit_function, :check_node + alias_method :visit_media, :check_node + alias_method :visit_mixindef, :check_node + alias_method :visit_mixin, :check_node + alias_method :visit_script_funcall, :check_node + + def feel_for_parens_and_check_node(node) + source = feel_for_enclosing_parens(node) + check(node, source) + yield + end + + alias_method :visit_script_listliteral, :feel_for_parens_and_check_node + alias_method :visit_script_mapliteral, :feel_for_parens_and_check_node + alias_method :visit_script_operation, :feel_for_parens_and_check_node + alias_method :visit_script_string, :feel_for_parens_and_check_node + + private + + TRAILING_WHITESPACE = /\s*$/ + + def check(node, source) # rubocop:disable Metrics/MethodLength @spaces = config['spaces'] + source = trim_right_paren(source) + return if source.count('(') != source.count(')') + source.scan(/ + \( + (?\s*) + (?.*) + (?\s*) + \) + /x) do |left, contents, right| + right = contents.match(TRAILING_WHITESPACE)[0] + right + contents.gsub(TRAILING_WHITESPACE, '') + + # We don't lint on multiline parenthetical source. + break if (left + contents + right).include? "\n" - engine.lines.each_with_index do |line, index| - line.gsub(%r{((//|/\*).*$)}, '').scan(/ - (^(\t|\s)*\))? # Capture leading spaces and tabs followed by a `)` - ( - \([ ]*(?!$) # Find `( ` as long as its not EOL ) - | - [ ]*\) - )? - /x) do |match| - check(match[2], index) if match[2] + if contents.empty? + # If we're looking at empty parens (like `()`, `( )`, `( )`, etc.), + # only report a possible lint on the left side. + right = ' ' * @spaces + end + + if left != ' ' * @spaces + message = "#{expected_spaces} after `(` instead of `#{left}`" + add_lint(node, message) + end + + if right != ' ' * @spaces + message = "#{expected_spaces} before `)` instead of `#{right}`" + add_lint(node, message) end end - yield end - private + # An expression enclosed in parens will include or not include each paren, depending + # on whitespace. Here we feel out for enclosing parens, and return them as the new + # source for the node. + def feel_for_enclosing_parens(node) # rubocop:disable Metrics/CyclomaticComplexity + range = node.source_range + original_source = source_from_range(range) + left_offset = -1 + right_offset = 0 + + if original_source[-1] != ')' + right_offset += 1 while character_at(range.end_pos, right_offset) =~ /\s/ - def check(str, index) - spaces = str.count ' ' - return if spaces == @spaces + return original_source if character_at(range.end_pos, right_offset) != ')' + end + + # At this point, we know that we're wrapped on the right by a ')'. + # Are we wrapped on the left by a '('? + left_offset -= 1 while character_at(range.start_pos, left_offset) =~ /\s/ + return original_source if character_at(range.start_pos, left_offset) != '(' + + # At this point, we know we're wrapped on both sides by parens. However, + # those parens may be part of a parent function call. We don't care about + # such parens. This depends on whether the preceding character is part of + # a function name. + return original_source if character_at(range.start_pos, left_offset - 1) =~ /[A-Za-z0-9_]/ + + range.start_pos.offset += left_offset + range.end_pos.offset += right_offset + source_from_range(range) + end + + # An unrelated right paren will sneak into the source of a node if there is no + # whitespace between the node and the right paren. + def trim_right_paren(source) + (source.count(')') == source.count('(') + 1) ? source[0..-2] : source + end - location = Location.new(index + 1) - message = "Expected #{pluralize(@spaces, 'space')} " \ - "between parentheses instead of #{spaces}" - add_lint(location, message) + def expected_spaces + "Expected #{pluralize(@spaces, 'space')}" end end end diff --git a/lib/scss_lint/sass/script.rb b/lib/scss_lint/sass/script.rb index 8f19ff99..b47274bb 100644 --- a/lib/scss_lint/sass/script.rb +++ b/lib/scss_lint/sass/script.rb @@ -74,5 +74,24 @@ def children [value] end end + + # This monkey patch can be removed once scss-lint depends on a minimum + # version of the sass gem that includes a fix for + # https://github.com/sass/sass/issues/1799 + class ListLiteral + def source_range + return @source_range if @elements.empty? + @source_range.end_pos = @elements.last.source_range.end_pos + @source_range + end + end + + class MapLiteral + def source_range + return @source_range if @pairs.empty? + @source_range.end_pos = @pairs.last.last.source_range.end_pos + @source_range + end + end end end diff --git a/spec/scss_lint/linter/space_between_parens_spec.rb b/spec/scss_lint/linter/space_between_parens_spec.rb index 976dc792..131c16de 100644 --- a/spec/scss_lint/linter/space_between_parens_spec.rb +++ b/spec/scss_lint/linter/space_between_parens_spec.rb @@ -119,6 +119,26 @@ it { should_not report_lint } end + context 'when a 0-argument function has nothing between parens' do + let(:scss) { <<-SCSS } + p { + property: unique-id(); + } + SCSS + + it { should_not report_lint } + end + + context 'when a 0-argument function has space between parens' do + let(:scss) { <<-SCSS } + p { + property: unique-id( ); + } + SCSS + + it { should report_lint line: 2 } + end + context 'when parens exist in a silent comment' do let(:scss) { <<-SCSS } p { @@ -132,13 +152,167 @@ context 'when parens exist in an outputted comment' do let(:scss) { <<-SCSS } p { - margin: 0; /* Some comment ( with parens ) */ + /* + * Some comment ( with parens ) + */ + margin: 0; } SCSS it { should_not report_lint } end + context 'when an import uses parens' do + let(:scss) { <<-SCSS } + @import url( foo ); + @import url(bar); + SCSS + + it { should report_lint line: 1, count: 2 } + end + + context 'when a variable definition uses parens' do + let(:scss) { <<-SCSS } + $family: unquote( "Droid+Sans" ); + $family: unquote("Droid+Sans"); + SCSS + + it { should report_lint line: 1, count: 2 } + end + + context 'when a variable definition is wrapped in parens' do + let(:scss) { <<-SCSS } + $value-map: (text: #00ff00, background: #0000ff, border: #ff0000); + $value-map: ( text: #00ff00, background: #0000ff, border: #ff0000 ); + SCSS + + it { should report_lint line: 2, count: 2 } + end + + context 'when a url uses parens' do + let(:scss) { <<-SCSS } + p { + background-image: url( 'bg1.png' ); + } + + code { + background-image: url('bg2.png'); + } + SCSS + + it { should report_lint line: 2, count: 2 } + end + + context 'when a @media directive uses parens' do + let(:scss) { <<-SCSS } + @media screen and (orientation: landscape) { + // Stuff. + } + + @media screen and ( orientation: landscape ) { + // Stuff. + } + SCSS + + it { should report_lint line: 5, count: 2 } + end + + context 'when an @at-root directive uses parens' do + let(:scss) { <<-SCSS } + @media screen { + @at-root (without: media) { + // Stuff. + } + @at-root ( with: media ) { + // Stuff. + } + } + SCSS + + it { should report_lint line: 5, count: 2 } + end + + context 'when an @if directive uses parens' do + let(:scss) { <<-SCSS } + p { + @if (1 + 2 == 2) { border: 1px; } + + @if ( 1 + 2 == 2 ) { border: 1px; } + } + SCSS + + it { should report_lint line: 4, count: 2 } + end + + context 'when an @each directive uses parens around a list' do + let(:scss) { <<-SCSS } + @each $animal in (puma, sea-slug, egret) { + // Stuff. + } + + @each $animal in ( puma, sea-slug, egret ) { + // Stuff. + } + SCSS + + it { should report_lint line: 5, count: 2 } + end + + context 'when an @each directive uses parens' do + let(:scss) { <<-SCSS } + /*@each $color, $cursor in (black, default), (blue, pointer) { + // Stuff. + }*/ + + @each $color, $cursor in ( black, default ), ( blue, pointer ) { + // Stuff. + } + SCSS + + it { should report_lint line: 5, count: 4 } + end + + context 'when a @mixin uses parens' do + let(:scss) { <<-SCSS } + @mixin sexy-border($color, $width) { + // Stuff. + } + + @mixin rockin-border( $color, $width ) { + // Stuff. + } + SCSS + + it { should report_lint line: 5, count: 2 } + end + + context 'when an @include uses parens' do + let(:scss) { <<-SCSS } + @mixin sexy-border($color, $width) { + // Stuff. + } + + a { @include sexy-border(blue, 1in); } + p { @include sexy-border( blue, 1in ); } + SCSS + + it { should report_lint line: 6, count: 2 } + end + + context 'when a @function uses parens' do + let(:scss) { <<-SCSS } + @function grid-width($n) { + @return $n * 2; + } + + @function grid-height( $n ) { + @return $n * 2; + } + SCSS + + it { should report_lint line: 5, count: 2 } + end + context 'when the number of spaces has been explicitly set' do let(:linter_config) { { 'spaces' => 1 } } @@ -259,5 +433,25 @@ it { should_not report_lint } end + + context 'when a 0-argument function has nothing between parens' do + let(:scss) { <<-SCSS } + p { + property: unique-id(); + } + SCSS + + it { should report_lint line: 2 } + end + + context 'when a 0-argument function has one space between parens' do + let(:scss) { <<-SCSS } + p { + property: unique-id( ); + } + SCSS + + it { should_not report_lint } + end end end