Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Support libsass string quotes in the c/js api bindings. #1006

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chriseppstein
Copy link
Contributor

This code compiles but the tests fail and I'm not sure why -- I think it might be a libsass bug. I could use some help debugging it.

@chriseppstein
Copy link
Contributor Author

@matryo @mgreter @xzyfer @saper any ideas about how to make this work? It's blocking us here at work I can't wait for the big refactor.

@saper
Copy link
Member

saper commented Jun 20, 2015

One reason why it fails is because quoted flag gets discarded from time to time...

@xzyfer
Copy link
Contributor

xzyfer commented Jun 20, 2015

I'm jumping on a plane to SF in a couple hours, I'll take a look at a
sensible hour today.

Regards,
Michael

On Fri, Jun 19, 2015 at 9:56 PM, Marcin Cieślak [email protected]
wrote:

One reason why it fails because quoted flag gets discarded from time to
time...


Reply to this email directly or view it on GitHub
#1006 (comment).

@saper
Copy link
Member

saper commented Jun 20, 2015

Well, basically quoted string support in libsass for custom functions is not implemented...

}
});

assert.equal(result.css.toString().trim(), '.swapped { result: asdf, "qwerty"; }');
Copy link
Member

Choose a reason for hiding this comment

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

Is this result: .swapped {\n result: asdf, "qwerty"; } acceptable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that seems right.

@saper
Copy link
Member

saper commented Jun 20, 2015

C SASS API kind of expects that a quoted string will be, ... well, quoted.

So it's not just a flag to be set, one actually needs to convert the mystring to "mystring" or 'mystring'.

@saper
Copy link
Member

saper commented Jun 20, 2015

I have prepared an early set of changes to libsass to make quoted strings work:

  1. Modifications to the 'should handle quoted strings' test chriseppstein/node-sass#1 modifies the test to provide an actual quoted string from the client (based on how currently API is constructed),

  2. https://github.com/saper/libsass/tree/clonefix fixes passing of quoted strings via the C API in libsass.

@saper
Copy link
Member

saper commented Jun 22, 2015

@chriseppstein Can you try applying sass/libsass#1289 to libsass? May not fix all quoted-vs-unquoted string issues, but certainly lets us pass them over to node and back.

@mgreter
Copy link
Contributor

mgreter commented Jun 23, 2015

@chriseppstein @saper IMO the merged PR on libsass side done by @saper does the correct thing. I can also give you a little background info why we have two string types (ATM). Strings that appear in sass are not always unquoted. There are "instances" that will always remain static (String_Constant) up to the output (preserving white-space and "comments" too). That's probably why you always want to deal with String_Quoted on the C-API side (looks like it simply never got implemented).

String_Quoted should work pretty straight forward from there on. You pass the "parsed" string to the constructor and it will be unquoted internally (storing the unquoted value and the quotemark flag). As a side note, I'm getting more confident that we could use a bool flag for the quotemark. We previousely had wrongly parsed some String_Constants as String_Quotes, which made it necessary to preserve the actual quotemark to pass the specs. Now we parse more of them correctly as String_Constant and should be able to get rid of explicitly storing the quotemark char, and instead always rely on autoquote rules. To get autoquote behavior (meaning we detect the best quotechar), you can set quotemark char to *.

@saper
Copy link
Member

saper commented Jun 23, 2015

Ah now I start to understand. So maybe "String_Quoted" should not be derived class of String_Constant? Are they in some circumstances interchangeable? Because in that case we should use common base class (I can see there is "String")... need to read more...

@saper saper force-pushed the master branch 2 times, most recently from 6c128d9 to 1e4bba8 Compare April 19, 2016 21:45
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this pull request Apr 7, 2024
Code: Fixes dynamic exception as per C++11 specs
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants