Don't raise error when template is empty#31
Conversation
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
How about return @template.dup if @template.empty?
There was a problem hiding this comment.
I just think '' makes it obvious we are returning an empty string...
There was a problem hiding this comment.
Perhaps, but it doesn't preserve the encoding of the input template.
There was a problem hiding this comment.
maybe that test could be adapted for taking the empty string into account, since the encoding shouldn't matter for empty strings...
Don't raise error when template is empty
|
Thanks for merging, @rosenfeld has a point about mutating the input. I think both solutions work, however 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. |
|
@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! |
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:
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.