From b0a9103d389c647324e84519c5306d35489d8630 Mon Sep 17 00:00:00 2001 From: Nathan Weizenbaum Date: Sat, 11 Jul 2009 16:40:23 -0400 Subject: [PATCH 1/3] [Sass] Failing test for a cache bug. --- test/sass/plugin_test.rb | 24 ++++++++++++++++++++++-- test/sass/templates/import.sass | 2 +- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/test/sass/plugin_test.rb b/test/sass/plugin_test.rb index 69513fa6..6dd35097 100644 --- a/test/sass/plugin_test.rb +++ b/test/sass/plugin_test.rb @@ -34,7 +34,7 @@ class SassPluginTest < Test::Unit::TestCase end def test_update_needed_when_modified - sleep(1) + sleep 1 FileUtils.touch(template_loc('basic')) assert Sass::Plugin.stylesheet_needs_update?('basic', template_loc, tempfile_loc) Sass::Plugin.update_stylesheets @@ -42,7 +42,7 @@ class SassPluginTest < Test::Unit::TestCase end def test_update_needed_when_dependency_modified - sleep(1) + sleep 1 FileUtils.touch(template_loc('basic')) assert Sass::Plugin.stylesheet_needs_update?('import', template_loc, tempfile_loc) Sass::Plugin.update_stylesheets @@ -122,6 +122,21 @@ class SassPluginTest < Test::Unit::TestCase assert !File.exists?(tempfile_loc('_partial')) end + ## Regression + + def test_cached_dependencies_update + FileUtils.mv(template_loc("basic"), template_loc("basic", "more_")) + set_plugin_opts :load_paths => [result_loc, template_loc(nil, "more_")] + + sleep 1 + FileUtils.touch(template_loc("basic", "more_")) + assert Sass::Plugin.stylesheet_needs_update?("import", template_loc, tempfile_loc) + Sass::Plugin.update_stylesheets + assert_renders_correctly("import") + ensure + FileUtils.mv(template_loc("basic", "more_"), template_loc("basic")) + end + private def assert_renders_correctly(*arguments) @@ -182,6 +197,11 @@ class SassPluginTest < Test::Unit::TestCase :always_update => true, }.merge(overrides) end + + def wait_a_tick + time = Time.now + loop {break if Time.now.sec != time.sec} + end end module Sass::Plugin diff --git a/test/sass/templates/import.sass b/test/sass/templates/import.sass index b4bfb121..1c76dc62 100644 --- a/test/sass/templates/import.sass +++ b/test/sass/templates/import.sass @@ -3,7 +3,7 @@ =premixin pre-mixin: here -@import importee, basic, basic.css, ../results/complex.css, partial +@import importee.sass, basic.sass, basic.css, ../results/complex.css, partial.sass nonimported :myconst = !preconst From 56c1e32b10f19d1905bb72d90123be2bb1cd7465 Mon Sep 17 00:00:00 2001 From: Nathan Weizenbaum Date: Sun, 12 Jul 2009 10:05:04 -0400 Subject: [PATCH 2/3] [Sass] Make the exception tests into separate test cases. --- test/sass/engine_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/sass/engine_test.rb b/test/sass/engine_test.rb index dea7d660..38d7041b 100755 --- a/test/sass/engine_test.rb +++ b/test/sass/engine_test.rb @@ -126,8 +126,8 @@ class SassEngineTest < Test::Unit::TestCase render("p\n\ta: b\n\tq\n\t\tc: d\n")) end - def test_exceptions - EXCEPTION_MAP.each do |key, value| + EXCEPTION_MAP.each do |key, value| + define_method("test_exception (#{key.inspect})") do line = 10 begin Sass::Engine.new(key, :filename => __FILE__, :line => line).render From 4b5842207fa9d07290535da126de09b310b8cc46 Mon Sep 17 00:00:00 2001 From: Nathan Weizenbaum Date: Sun, 12 Jul 2009 10:21:21 -0400 Subject: [PATCH 3/3] [Sass] Move import resolution to ImportNode (formerly FileNode). Closes gh-17 --- lib/sass/engine.rb | 52 ++++++------------- lib/sass/tree/directive_node.rb | 5 +- .../tree/{file_node.rb => import_node.rb} | 34 ++++++++++-- 3 files changed, 46 insertions(+), 45 deletions(-) rename lib/sass/tree/{file_node.rb => import_node.rb} (52%) diff --git a/lib/sass/engine.rb b/lib/sass/engine.rb index a2bc98b8..09357c7e 100644 --- a/lib/sass/engine.rb +++ b/lib/sass/engine.rb @@ -12,7 +12,7 @@ require 'sass/tree/if_node' require 'sass/tree/while_node' require 'sass/tree/for_node' require 'sass/tree/debug_node' -require 'sass/tree/file_node' +require 'sass/tree/import_node' require 'sass/environment' require 'sass/script' require 'sass/error' @@ -227,21 +227,23 @@ END def build_tree(parent, line, root = false) @line = line.index - node = parse_line(parent, line, root) + node_or_nodes = parse_line(parent, line, root) - # Node is a symbol if it's non-outputting, like a variable assignment, - # or an array if it's a group of nodes to add - return node unless node.is_a? Tree::Node + Array(node_or_nodes).each do |node| + # Node is a symbol if it's non-outputting, like a variable assignment + next unless node.is_a? Tree::Node - node.line = line.index - node.filename = line.filename + node.line = line.index + node.filename = line.filename - if node.is_a?(Tree::CommentNode) - node.lines = line.children - else - append_children(node, line.children, false) + if node.is_a?(Tree::CommentNode) + node.lines = line.children + else + append_children(node, line.children, false) + end end - return node + + node_or_nodes end def append_children(parent, children, root) @@ -279,7 +281,7 @@ END case child when Tree::MixinDefNode raise SyntaxError.new("Mixins may only be defined at the root of a document.", line.index) - when Tree::DirectiveNode, Tree::FileNode + when Tree::ImportNode raise SyntaxError.new("Import directives may only be used at the root of a document.", line.index) end end @@ -365,7 +367,7 @@ END # it's a CSS @import rule and we don't want to touch it. if directive == "import" && value !~ /^(url\(|")/ raise SyntaxError.new("Illegal nesting: Nothing may be nested beneath import directives.", @line + 1) unless line.children.empty? - import(value) + value.split(/,\s*/).map {|f| Tree::ImportNode.new(f)} elsif directive == "for" parse_for(line, root, value) elsif directive == "else" @@ -470,27 +472,5 @@ END offset = options[:offset] || 0 Script.parse(script, line, offset, @options[:filename]) end - - def import_paths - paths = (@options[:load_paths] || []).dup - paths.unshift(File.dirname(@options[:filename])) if @options[:filename] - paths - end - - def import(files) - files.split(/,\s*/).map do |filename| - engine = nil - - begin - filename = Sass::Files.find_file_to_import(filename, import_paths) - rescue Exception => e - raise SyntaxError.new(e.message, @line) - end - - next Tree::DirectiveNode.new("@import url(#{filename})") if filename =~ /\.css$/ - - Tree::FileNode.new(filename) - end.flatten - end end end diff --git a/lib/sass/tree/directive_node.rb b/lib/sass/tree/directive_node.rb index f7f09b25..0034d5d0 100644 --- a/lib/sass/tree/directive_node.rb +++ b/lib/sass/tree/directive_node.rb @@ -5,10 +5,7 @@ module Sass::Tree # only CSS directives like `@media` and `@font-face` become {DirectiveNode}s. # # `@import` is a bit of a weird case; - # if it's importing a Sass file, - # it becomes a {FileNode}, - # but if it's importing a plain CSS file, - # it becomes a {DirectiveNode}. + # it becomes an {ImportNode}. # # @see Sass::Tree class DirectiveNode < Node diff --git a/lib/sass/tree/file_node.rb b/lib/sass/tree/import_node.rb similarity index 52% rename from lib/sass/tree/file_node.rb rename to lib/sass/tree/import_node.rb index 29447f06..4ff03415 100644 --- a/lib/sass/tree/file_node.rb +++ b/lib/sass/tree/import_node.rb @@ -3,10 +3,10 @@ module Sass # A static node that wraps the {Sass::Tree} for an `@import`ed file. # It doesn't have a functional purpose other than to add the `@import`ed file # to the backtrace if an error occurs. - class FileNode < Node - # @param filename [String] The name of the imported file - def initialize(filename) - @filename = filename + class ImportNode < Node + # @param imported_filename [String] The name of the imported file + def initialize(imported_filename) + @imported_filename = imported_filename super() end @@ -30,12 +30,36 @@ module Sass # @param environment [Sass::Environment] The lexical environment containing # variable and mixin values def perform!(environment) - self.children = Sass::Files.tree_for(filename, @options).children + return unless full_filename = import + self.children = Sass::Files.tree_for(full_filename, @options).children self.children = perform_children(environment) rescue Sass::SyntaxError => e e.add_backtrace_entry(@filename) raise e end + + private + + def import_paths + paths = (@options[:load_paths] || []).dup + paths.unshift(File.dirname(@options[:filename])) if @options[:filename] + paths + end + + def import + begin + full_filename = Sass::Files.find_file_to_import(@imported_filename, import_paths) + rescue Exception => e + raise SyntaxError.new(e.message, self.line) + end + + if full_filename =~ /\.css$/ + @to_s = "@import url(#{full_filename});" + return false + end + + return full_filename + end end end end