Add column and length information to Lint

As noted in id_with_extraneous_selector.rb,
Sass::Selector::SimpleSequence#source_range sometimes lies about its
line. So for now the linter reports the line number with
SimpleSequence#line and discards column and length information.

Change-Id: Ibc71b5662a0797046230287a4ff40b61fffb01a0
Reviewed-on: http://gerrit.causes.com/37222
Tested-by: jenkins <[email protected]>
Reviewed-by: Shane da Silva <[email protected]>

Author: yujinakayama

Rakefile

@@ -0,0 +1 @@
+require 'bundler/gem_tasks'

lib/scss_lint.rb

@@ -2,6 +2,7 @@
 require 'scss_lint/config'
 require 'scss_lint/cli'
 require 'scss_lint/engine'
+require 'scss_lint/location'
 require 'scss_lint/lint'
 require 'scss_lint/linter_registry'
 require 'scss_lint/runner'

lib/scss_lint/cli.rb

@@ -181,7 +181,7 @@ def scssish_file?(file)
 
     # @param lints [Array<Lint>]
     def report_lints(lints)
-      sorted_lints = lints.sort_by { |l| [l.filename, l.line] }
+      sorted_lints = lints.sort_by { |l| [l.filename, l.location] }
       reporter = @options.fetch(:reporter, Reporter::DefaultReporter)
                          .new(sorted_lints)
       output = reporter.report_lints

lib/scss_lint/lint.rb

@@ -1,15 +1,15 @@
 module SCSSLint
   # Stores information about a single problem that was detected by a [Linter].
   class Lint
-    attr_reader :filename, :line, :description, :severity
+    attr_reader :filename, :location, :description, :severity
 
     # @param filename [String]
-    # @param line [Integer]
+    # @param location [SCSSLint::Location]
     # @param description [String]
     # @param severity [Symbol]
-    def initialize(filename, line, description, severity = :warning)
+    def initialize(filename, location, description, severity = :warning)
       @filename    = filename
-      @line        = line
+      @location    = location
       @description = description
       @severity    = severity
     end

lib/scss_lint/linter.rb

@@ -23,13 +23,34 @@ def run(engine, config)
     # @param node_or_line [Sass::Script::Tree::Node, Sass::Engine::Line]
     # @param message [String]
     def add_lint(node_or_line, message)
-      line = node_or_line.respond_to?(:line) ? node_or_line.line : node_or_line
+      location = if node_or_line.respond_to?(:source_range) && node_or_line.source_range
+                   location_from_range(node_or_line.source_range)
+                 elsif node_or_line.respond_to?(:line)
+                   Location.new(node_or_line.line)
+                 else
+                   Location.new(node_or_line)
+                 end
 
       @lints << Lint.new(engine.filename,
-                         line,
+                         location,
                          message)
     end
 
+    # Helper for creating location from a source range
+    #
+    # @param range [Sass::Source::Range]
+    # @return [SCSSLint::Location]
+    def location_from_range(range)
+      length = if range.start_pos.line == range.end_pos.line
+                 range.end_pos.offset - range.start_pos.offset
+               else
+                 line_source = engine.lines[range.start_pos.line]
+                 line_source.length - range.start_pos.offset + 1
+               end
+
+      Location.new(range.start_pos.line, range.start_pos.offset, length)
+    end
+
     # @param source_position [Sass::Source::Position]
     # @param offset [Integer]
     # @return [String] the character at the given [Sass::Source::Position]

lib/scss_lint/linter/id_with_extraneous_selector.rb

@@ -13,8 +13,10 @@ def visit_simple_sequence(seq)
       end
 
       if can_be_simplified
-        add_lint(seq, "Selector `#{seq}` can be simplified to `#{id_sel}`, " \
-                      'since IDs should be uniquely identifying')
+        # TODO: Sass::Selector::SimpleSequence#source_range sometimes lies about
+        # its line, so reference `#line` directly
+        add_lint(seq.line, "Selector `#{seq}` can be simplified to `#{id_sel}`, " <<
+                           'since IDs should be uniquely identifying')
       end
     end
   end

lib/scss_lint/linter/space_between_parens.rb

@@ -26,8 +26,10 @@ def check(str, index, engine)
       spaces = str.count ' '
 
       if spaces != @spaces
-        @lints << Lint.new(engine.filename, index + 1, "Expected #{pluralize(@spaces, 'space')} " \
-                                                       "between parentheses instead of #{spaces}")
+        location = Location.new(index + 1)
+        message = "Expected #{pluralize(@spaces, 'space')}" <<
+                  " between parentheses instead of #{spaces}"
+        @lints << Lint.new(engine.filename, location, message)
       end
     end
   end

lib/scss_lint/location.rb

@@ -0,0 +1,38 @@
+module SCSSLint
+  # Stores a location of [Lint] in a source.
+  class Location
+    include Comparable
+
+    attr_reader :line, :column, :length
+
+    # @param line [Integer] An one-based index
+    # @param column [Integer] An one-based index
+    # @param length [Integer] A length in the line
+    def initialize(line = 1, column = 1, length = 1)
+      raise ArgumentError, "Line must be more than 0, passed #{line}" if line < 1
+      raise ArgumentError, "Column must be more than 0, passed #{column}" if column < 1
+      raise ArgumentError, "Length must be more than 0, passed #{length}" if length < 1
+
+      @line   = line
+      @column = column
+      @length = length
+    end
+
+    def ==(other)
+      [:line, :column, :length].all? do |attr|
+        send(attr) == other.send(attr)
+      end
+    end
+
+    alias_method :eql?, :==
+
+    def <=>(other)
+      [:line, :column, :length].each do |attr|
+        result = send(attr) <=> other.send(attr)
+        return result unless result == 0
+      end
+
+      0
+    end
+  end
+end

lib/scss_lint/reporter/default_reporter.rb

@@ -5,7 +5,7 @@ def report_lints
       if lints.any?
         lints.map do |lint|
           type = lint.error? ? '[E]'.color(:red) : '[W]'.color(:yellow)
-          "#{lint.filename.color(:cyan)}:" << "#{lint.line}".color(:magenta) <<
+          "#{lint.filename.color(:cyan)}:" << "#{lint.location.line}".color(:magenta) <<
                                       " #{type} #{lint.description}"
         end.join("\n") + "\n"
       end

lib/scss_lint/reporter/xml_reporter.rb

@@ -8,9 +8,8 @@ def report_lints
       lints.group_by(&:filename).each do |filename, file_lints|
         output << "<file name=#{filename.encode(:xml => :attr)}>"
 
-
         file_lints.each do |lint|
-          output << "<issue line=\"#{lint.line}\" " <<
+          output << "<issue line=\"#{lint.location.line}\" " <<
                            "severity=\"#{lint.severity}\" " <<
                            "reason=#{lint.description.encode(:xml => :attr)} />"
         end

lib/scss_lint/runner.rb

@@ -49,9 +49,9 @@ def find_lints(file)
         end
       end
     rescue Sass::SyntaxError => ex
-      @lints << Lint.new(ex.sass_filename, ex.sass_line, ex.to_s, :error)
+      @lints << Lint.new(ex.sass_filename, Location.new(ex.sass_line), ex.to_s, :error)
     rescue FileEncodingError => ex
-      @lints << Lint.new(file, 1, ex.to_s, :error)
+      @lints << Lint.new(file, Location.new, ex.to_s, :error)
     end
   end
 end

spec/scss_lint/location_spec.rb

@@ -0,0 +1,42 @@
+require 'spec_helper'
+
+describe SCSSLint::Location do
+  let(:engine) { described_class.new(css) }
+
+  describe '#<=>' do
+    let(:locations) do
+      [
+        SCSSLint::Location.new(2, 2, 2),
+        SCSSLint::Location.new(2, 2, 1),
+        SCSSLint::Location.new(2, 1, 2),
+        SCSSLint::Location.new(2, 1, 1),
+        SCSSLint::Location.new(1, 2, 2),
+        SCSSLint::Location.new(1, 2, 1),
+        SCSSLint::Location.new(1, 1, 2),
+        SCSSLint::Location.new(1, 1, 1)
+      ]
+    end
+
+    it 'enables to sort locations' do
+      locations.sort.should == [
+        SCSSLint::Location.new(1, 1, 1),
+        SCSSLint::Location.new(1, 1, 2),
+        SCSSLint::Location.new(1, 2, 1),
+        SCSSLint::Location.new(1, 2, 2),
+        SCSSLint::Location.new(2, 1, 1),
+        SCSSLint::Location.new(2, 1, 2),
+        SCSSLint::Location.new(2, 2, 1),
+        SCSSLint::Location.new(2, 2, 2)
+      ]
+    end
+
+    context 'when the same location is passed' do
+      let(:location) { SCSSLint::Location.new(1, 1, 1) }
+      let(:other_location) { SCSSLint::Location.new(1, 1, 1) }
+
+      it 'returns 0' do
+        (location <=> other_location).should == 0
+      end
+    end
+  end
+end

spec/scss_lint/reporter/default_reporter_spec.rb

@@ -19,7 +19,8 @@
       let(:severities)   { [:warning] * 2 }
       let(:lints) do
         filenames.each_with_index.map do |filename, index|
-          SCSSLint::Lint.new(filename, lines[index], descriptions[index], severities[index])
+          location = SCSSLint::Location.new(lines[index])
+          SCSSLint::Lint.new(filename, location, descriptions[index], severities[index])
         end
       end
 

spec/scss_lint/reporter/files_reporter_spec.rb

@@ -16,7 +16,7 @@
       let(:filenames)    { ['some-filename.scss', 'some-filename.scss', 'other-filename.scss'] }
       let(:lints) do
         filenames.each_with_index.map do |filename, index|
-          SCSSLint::Lint.new(filename, 0, '', :warning)
+          SCSSLint::Lint.new(filename, SCSSLint::Location.new, '', :warning)
         end
       end
 

spec/scss_lint/reporter/xml_reporter_spec.rb

@@ -35,7 +35,8 @@
       let(:severities)   { [:warning] * 3 }
       let(:lints) do
         filenames.each_with_index.map do |filename, index|
-          SCSSLint::Lint.new(filename, lines[index], descriptions[index], severities[index])
+          location = SCSSLint::Location.new(lines[index])
+          SCSSLint::Lint.new(filename, location, descriptions[index], severities[index])
         end
       end
 

spec/support/matchers/report_lint.rb

@@ -14,7 +14,7 @@
       when 0
         ''
       when 1
-        ", but was on line #{linter.lints.first.line}"
+        ", but was on line #{linter.lints.first.location.line}"
       else
         lines = lint_lines(linter)
         ", but lints were reported on lines #{lines[0...-1].join(', ')} and #{lines.last}"
@@ -43,6 +43,6 @@ def has_lints?(linter, expected_line, count)
   end
 
   def lint_lines(linter)
-    linter.lints.map(&:line)
+    linter.lints.map { |lint| lint.location.line }
   end
 end