Allow optional params #7
Conversation
|
|
||
| value = functs.send(custom_function, s) | ||
| custom_function_arguments << argument |
There was a problem hiding this comment.
do we want to add an else here that raises something like NotImplemented for now?
There was a problem hiding this comment.
For values that aren’t a :sass_string? Yeah, that’s a good idea.
I also kinda want to just implement the rest 😄
It looks like the type classes like SassC:Script:String look just like their Sass versions, but with an added #to_native and a couple other tweaks.
How would you feel about temporarily requiring the Sass counterparts like https://github.com/sass/sass/blob/stable/lib/sass/script/value/string.rb and inheriting + extending/overriding those classes? Then we wouldn’t fall behind as Sass grows / our copy-pasted version stays static.
There was a problem hiding this comment.
it's unfortunate to have the sass dependency long term, but right at the moment, sassc-rails requires sass too (because sprockets requires sass). I suppose that would be a good solution for the present.
I can't exactly remember, but i think I may have had to tweak the String value class (i.e. it might not be exactly the same as it is in sass).
There was a problem hiding this comment.
Purely for informational purposes, and not to add an opinion or bias one way or the other, we did a diff. The only things that you seem to have changed are:
- removed a
supercall from the constructor - added
self.typeandself.unquoteclass methods - added
#to_native - removed
#plus,#to_sass, and#inspect. Or maybe those just didn't exist yet?
There was a problem hiding this comment.
Aside from that, both keeping sass as a dependency and copy/pasting the Sass::Script counterparts feel equally dirty to me :(
There was a problem hiding this comment.
do we want to add an else here that raises something like NotImplemented for now?
@bolandrm Since hooking into libsass’ own error handling stuff is a little too tricky for me at the moment, I’ve added some code that upon error, spits out a Sass syntax error and also logs to $stderr.
|
Looking good to me. thanks for the rakefile refactors. |
| # ADDAPI char* ADDCALL sass_error_get_message (const union Sass_Value* v); | ||
| # ADDAPI void ADDCALL sass_error_set_message (union Sass_Value* v, char* msg); | ||
| attach_function :sass_error_get_message, [:sass_value_ptr], :string | ||
| attach_function :sass_error_set_message, [:sass_value_ptr, :pointer], :void |
edward
force-pushed
the
allow-optional-params
branch
from
b43b733 to
5806d11
Compare
edward
force-pushed
the
allow-optional-params
branch
from
5806d11 to
4b4d324
Compare
|
@bolandrm we cool to merge on green? |
Fix create link
Adds support for:
Bonus:
@nickjs @bolandrm for review