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

Already on GitHub? Sign in to your account

Allowed extensions #1312

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Owner

scottdavis commented Jun 5, 2013

Allows sprite engines to define allowed extensions

Owner

scottdavis commented Jun 5, 2013

This should allow #907

Owner

scottdavis commented Jul 23, 2013

fixes #1244

@chriseppstein chriseppstein commented on the diff Dec 3, 2013

lib/compass/sass_extensions/sprites/engines.rb
module Compass
module SassExtensions
module Sprites
class Engine
attr_accessor :width, :height, :images, :canvas
- def initialize(width, height, images)
+ def initialize(width=nil, height=nil, images=nil)
@chriseppstein

chriseppstein Dec 3, 2013

Owner

what is this change for?

@scottdavis

scottdavis Dec 3, 2013

Owner

no idea was 4 months ago :(

@chriseppstein chriseppstein commented on the diff Dec 3, 2013

lib/compass/sass_extensions/sprites/sprite_map.rb
@modulize ||= Compass::configuration.sprite_engine.to_s.scan(/([^_.]+)/).flatten.map {|chunk| "#{chunk[0].chr.upcase}#{chunk[1..-1]}" }.join
end
+ def self.constantize(camel_cased_word)
+ names = camel_cased_word.split('::')
+ names.shift if names.empty? || names.first.empty?
+
+ constant = Object
+ names.each do |name|
+ constant = constant.const_defined?(name) ? constant.const_get(name) : constant.const_missing(name)
+ end
+ constant
+ end
+
+
+
@chriseppstein

chriseppstein Dec 3, 2013

Owner

neeeeeewlineeeeees.

@chriseppstein chriseppstein commented on the diff Dec 3, 2013

lib/compass/sprite_importer.rb
require 'compass/sprite_importer/binding'
+
+
+
+
@chriseppstein

chriseppstein Dec 3, 2013

Owner

neeeeeewlineeeeees!

@chriseppstein chriseppstein commented on the diff Dec 3, 2013

lib/compass/sass_extensions/sprites/sprite_methods.rb
@@ -18,7 +18,7 @@ def compute_image_metadata!
end
def init_engine
- @engine = eval("::Compass::SassExtensions::Sprites::#{modulize}Engine.new(nil, nil, nil)")
+ @engine = self.class.sprite_engine_class.new
@engine.width = @width
@engine.height = @height
@engine.images = @images
@chriseppstein

chriseppstein Dec 3, 2013

Owner

Why not just pass these arguments to the initializer instead of making them optional?

@chriseppstein chriseppstein commented on the diff Dec 3, 2013

lib/compass/sprite_importer.rb
module Compass
class SpriteImporter < Sass::Importers::Base
VAILD_FILE_NAME = /\A#{Sass::SCSS::RX::IDENT}\Z/
- SPRITE_IMPORTER_REGEX = %r{((.+/)?([^\*.]+))/(.+?)\.png}
- VALID_EXTENSIONS = ['.png']
+ SPRITE_IMPORTER_REGEX = %r{((.+/)?([^\*.]+))/(.+?)}
@chriseppstein

chriseppstein Dec 3, 2013

Owner

I'm ok with relaxing this glob restriction, but unless the user specifies a valid extension in the glob explicitly, I think we should return nil from find unless we find an image with a valid extension in the folder. This will keep it from conflicting with globs that import sass.

@scottdavis

scottdavis Dec 3, 2013

Owner

So the entire point of this was to move the required extensions down a level to the engine classes so things like svg gif or whatever people wanted could be created and also allow me to move forward using the same code to work on a svg -> font glyph engine

@chriseppstein

chriseppstein Dec 3, 2013

Owner

That's fine. My point is that when the glob matches, we always return a sass engine. Instead, the sprite engine should be able to look at that glob and decide if it has files in it that it can process. If not, this importer should return nil and then a different importer can handle it. Or maybe Sass will just give an error that says that it cannot import that.

@scottdavis scottdavis commented on the diff Dec 3, 2013

lib/compass/sprite_importer.rb
@@ -107,6 +111,16 @@ def self.content_for_images(uri, name, skip_overrides = false)
binder = Compass::Sprites::Binding.new(:name => name, :uri => uri, :skip_overrides => skip_overrides, :sprite_names => sprite_names(uri), :files => files(uri))
CONTENT_TEMPLATE.result(binder.get_binding)
end
+
+ private
+
+ def self.valid_extensions
+ @valid_extensions ||= SassExtensions::Sprites::SpriteMap.sprite_engine_class::VALID_EXTENSIONS
+ end
+
+ def self.sprite_importer_regex_with_ext
+ @importer_regex ||= %r{#{SPRITE_IMPORTER_REGEX}(#{valid_extensions.join('|')})}
+ end
@scottdavis

scottdavis Dec 3, 2013

Owner

So here is where the glob is scoped.

@chriseppstein

chriseppstein Dec 3, 2013

Owner

I see. I'd still like to not return any engine if there's no files that match the glob.

Owner

chriseppstein commented Aug 18, 2014

I'm closing this PR because there's been too much change since it was initially submitted. Sorry, We're going to do better going forward. Hopefully, someone can resubmit the change against 1.0. Features should be submitted to the master branch, bug fixes to the stable branch.

Owner

chriseppstein commented Aug 18, 2014

Should be on top of the changes coming in #1771.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment