Correct handling of escaped interpolation \#{}

[smikes]

• partial merge of change to inspect.cpp
• initial set of catch unit tests
• add quote to Makefile.am
• remaining tests to cover quote
• escape backslash
• fix pin tests
• add more tests of current behavior
• factor out interpolant fns
• correct implementation of next_unescaped_interpolant
• unit tests for next_unescaped_interpolant
• ignore Emacs TAGS files

[coveralls]

Coverage increased (+0.06%) when pulling be25eea on smikes:escaped-interpolation into 31521ef on sass:master.

[smikes]

Here is a candidate PR for the first part of my work on quoting/interpolation in libsass.

See sass/sass-spec#218 for the spec.

Although the code in libsass that detects interpolations appears to ignore escaped ones:

find_first_in_interval< sequence< negate< exactly<'\\'> >, exactly<hash_lbrace> > >(b, e);

my unit tests showed that this did not work. Worked around it in the stupidest way possible, that is, with an explicit guard. May be amenable to cleanup later, but for now, this is a step that increases correctness and coverage (+0.06%)

[am11]

q is unused, isn't it? :)

[smikes]

Thanks. I missed this, because I did not re-inspect unquote after I handled the merge from #779 , commit 5483186.

[am11]

Oh nice. I was just checking appveyor build: https://ci.appveyor.com/project/sass/libsass/build/1.0.6. Not sure why mingw made it a matter of fatality and terminated tests for such a trivial reason (it should just be a compiler warning). msvc is also choking with unresolved symbol.

[smikes]

Is it running with an implicit 'warnings as errors' setting? Anyway, thanks for pointing it out. When 1.0.14 https://ci.appveyor.com/project/sass/libsass/build/1.0.14 finally runs we'll see if I got it.

[coveralls]

Coverage increased (+0.08%) when pulling 976d64a on smikes:escaped-interpolation into 31521ef on sass:master.

[coveralls]

Coverage increased (+0.08%) when pulling 1c2724a on smikes:escaped-interpolation into 31521ef on sass:master.

[mgreter]

Just wanted to give you some feedback on your work after a short review:

• I really like the refactoring in parser.cpp!
• If we move quote/unquote, I would move it to utils.cpp! *1
• I like that you used TDD, but we have some other issues with testing *2

*1: I guess we want to keep the fragmentation at least down to a certain level. I agree that seperation is a good principle, but it also increases compile time. I think quote/unquote would fit very well into utils.cpp!

*2: This is a bit more complicated and there have been already a lot of discussions about this. AFAIK we do not want to integrate another test framework without further considerations, so this seems IMHO not acceptable for this PR! Prefered solution in this case would be that you could re-create most of the test cases as spec-tests!

We definately want to add more and better testing and are open for further PRs, but we need quite a bit to cover all cases:

• output modes
• context options
• source-maps
• exit states
• error/warn output
• stderr output
• ...

Currently the core developers are mostly on bringing the code up to sass 3.4 parity! So the testing seems to get little attention. I use perl-libsass to test more advanced features that are not covered by the sass spec test suite (like my recent developement with source-maps).

To do this properly we will need to have a test-runner in C that can test all the above scenarios. Once we have that, it could also include more basic tests like the once you have in this PR!

[smikes]

Okay, I will move the quote/unquote functions into util.

I will remove the unit tests from this PR. I appreciate that nobody wants to add a new dependency, especially a random untried C++ unit test framework. However, I argue that unit tests are critical to this project, and in particular, for quoting/unquoting and similar internal functions that are never exposed. In this case, I was only able to implement a correct next_unescaped_interpolant() by using unit tests; and the refactoring was tricky because I could not establish unit tests to pin the behavior as I worked.

Catch provides everything I want in a modern test framework for C++, including selecting tests by name or by tag, various output formats, separate compilation etc. Please check it out, and if there is no consensus about a unit test framework but a general consensus that there ought to be more testing, I would volunteer to support Catch unit tests. https://github.com/philsquared/Catch

In any case, I will continue to maintain unit tests in my own branch and try to prevent them from leaking into my PRs.

[mgreter]

I agree that we are in desperate need of better testing. We do have a very good test-suite, which ensures pretty well that we do not break existing compiles. But when it comes down to tiny details, we are in lack of any testing! I also have my "own" tests in the perl-bindings to test stuff on the C-API which we do not currently test inside libsass itself. The same goes for the node-sass test-suite. But be aware that a sufficient test suite for libsass should include source-maps, exit-codes, error-msgs, warnings, importers and more!

We would (AFAIK) love to replace the current ruby spec runner with a native C++ implementation. This would IMO be a great opportunity for somebody who would like to get to know the libsass internals 😉

Edit: Just a wild idea, but I think you could put your C unit tests into an own repository with a Makefile and links against libsass!? That way we might could adopt it as an alternative test runner someday!?

[am11]

Regarding tests, libsass relies on sassc executable, which also acts like a driver to run sass-spec test suite, which uses Ruby as an intermediary. But we only achieve functional testing by this approach (on relying on tertiary agent libsass->sassc->ruby..).

Here is a related issue: sass/sassc#90. As @xzyfer mentioned, the Ruby entity in this test lifecycle is nothing but a convince at this point, and we can gradually move to C++ based testing framework, I think (and what @mgreter said) with that, we might be able to test more features such as options, source-maps and much more features, which are (not totally, but) unrelated to spec.

Having said all that, it would still be functional testing, satelliting the API with sassc (which is more native than throwing in Ruby/Python to the mix), as opposed to Unit test, which shall be more focused the nitty-gritty of implementation itself.

Here is my mind-map (not the graphical one 📈 ):

• [Functional] Sassc to load spec suites, filter-out to-dos tests and run the suites while maintain stats (a drop-in replacement to what Ruby script is doing there on sass-spec repo). IMO ruby script shouldn't live on sass-spec repo either. Spec repo should only have pairs of input and expected output. (FTR, I do not have anything against Ruby). :)
• [Functional] Sassc to test libsass' niche features such as; custom importer, custom functions, all those surrounding disableable options and last but not the least, source-maps.
• [Unit] In libsass, we can have Catch (or CppUnit) to test the components of code, and I believe @smikes has presented more than just a prove-of-concept! This is exactly what we ever wanted... isn't it? 😄

[coveralls]

Coverage increased (+0.13%) when pulling 8487824 on smikes:escaped-interpolation into 31521ef on sass:master.

[xzyfer]

I agree with @mgreter . I like the intent here but adding a new dependency isn't viable at this point in time.

There may very well be internal inconsistencies at times, but that doesn't matter as long as the output is correct for input supplied. All these use cases can and should be covered by sass spec.

[xzyfer]

As it stands I think this PR's commits need a tidy up. Two or three well written commits should get the job done whilst preserving appropriate forensic git information. I can assist with this is you need.

[smikes]

As it stands I think this PR's commits need a tidy up. Two or three well written commits should get the job done whilst preserving appropriate forensic git information. I can assist with this is you need.

Squashed down to one commit. I don't think there was anything important in the intervening commits, just add/revert of the new file quote.cpp and add/revert of the unit tests.

[mgreter]

Unfortunately most of these cases are also covered by #910, so I'm going to close this. Sorry @smikes, but if you see anything I have missed in that commit, feel free to open more PRs! Thank you!