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

Don't raise error when template is empty #31

Merged
merged 2 commits into from
Jan 11, 2016

Conversation

fschwahn
Copy link
Contributor

sassc raises an error when an empty template is passed (see https://github.com/sass/libsass/blob/b2a3d98ed384d2d19aa79157d08fa21c3e8043e0/src/sass_context.cpp#L390). rubysass accepts that however, that means the sass and sassc gems behave differently when passed the same input:

Sass::Engine.new('', style: :compressed).render # => ""
SassC::Engine.new('', style: :compressed).render # raises SassC::SyntaxError
# but with one space:
SassC::Engine.new(' ', style: :compressed).render # => ""

This change just short-circuits the whole Engine#render-method if the template is empty. This is the easiest solution I could see, but maybe I'm missing something.

A side note: While putting this together I noticed that the :quiet-option seems badly named, as it is more like a dry run if I understand it correctly. The quiet option in rubysass actually surpresses warnings, whule the sassc version just does not return the resulting css. This subtle difference might trip someone up. If the current behavior of the quiet option is indeed intentional, then this PR needs to be updated to make the behavior the same for the empty template case.

@@ -17,6 +17,8 @@ def initialize(template, options = {})
end

def render
return @template if @template.empty?

Choose a reason for hiding this comment

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

Wouldn't it be better to replace it with return '' if @template.empty?? It may seem the same but it isn't. With the current implementation we would get a reference to the original source and maybe change it after calling render causing a side effect on the original source which could cause some unexpected behaviors if one assume render would always return a new string...

Copy link
Member

Choose a reason for hiding this comment

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

How about return @template.dup if @template.empty?

Choose a reason for hiding this comment

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

I just think '' makes it obvious we are returning an empty string...

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

maybe that test could be adapted for taking the empty string into account, since the encoding shouldn't matter for empty strings...

bolandrm added a commit that referenced this pull request Jan 11, 2016
Don't raise error when template is empty
@bolandrm bolandrm merged commit ac003ab into sass:master Jan 11, 2016
@fschwahn
Copy link
Contributor Author

Thanks for merging, @rosenfeld has a point about mutating the input. I think both solutions work, however return ''.force_encoding(@template.encoding) if @template.empty? is probably more explicit than return @template.dup if @template.empty?.

It would probably make sense to add tests that the returned empty string is not the same as the input string, as well as that it preserves the encoding.

@bolandrm
Copy link
Member

@fschwahn I merged as some folks were running into this problem. If you want to further improve the code or tests, that's more than welcome - feel free to submit another PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants