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

Fix interpolation edge cases with escapes #1118

Merged
merged 2 commits into from
Apr 26, 2015

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented Apr 20, 2015

Fixes #1115

@mgreter mgreter self-assigned this Apr 20, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 80.32% when pulling 8e04865 on mgreter:bugfix/issue_1115 into f90a1b7 on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented Apr 20, 2015

👍

@xzyfer xzyfer added this to the 3.2 milestone Apr 20, 2015
@xzyfer
Copy link
Contributor

xzyfer commented Apr 23, 2015

I've created the spec for this issue - sass/sass-spec#340

This PR is an improvement on the current behaviour, however it still fails this spec.

@xzyfer
Copy link
Contributor

xzyfer commented Apr 26, 2015

So I've spent all weekend getting my head around our string handling code, and feel it's already over complicated. I'm super reluctant to add another flag to our String AST nodes i.e. String::sass_fix_1291

I wonder if we can tackle this case entirely in the Eval phase instead?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 80.29% when pulling 08a00b5 on mgreter:bugfix/issue_1115 into daf9163 on sass:master.

@mgreter
Copy link
Contributor Author

mgreter commented Apr 26, 2015

@xzyfer I fully agree, but the handling by ruby sass is pretty complex and AFAIR I was able to get rid of quite a few flags with my last refactoring. But I pretty much gave up to understand how ruby sass internally handles interpolations and escape sequences (We keep finding new edge cases all the time). So for the time beeing I really only see the way to implement specific code paths for these cases, until we have a good enough spec suite to tackle another refactoring. My last commit should solve the reported issues, but it is far away from beeing pretty. I'm having problems coming up with ideas how to name all those different escape/unescape functions, but I currently don't see a way to get rid of them.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 80.3% when pulling e771bc5 on mgreter:bugfix/issue_1115 into daf9163 on sass:master.

@mgreter
Copy link
Contributor Author

mgreter commented Apr 26, 2015

Btw. I have used this extended test case:

foo {
    content: "x\79";
    content: "#{x}\79";
    content: "x\0\1\2\3\4\5\6\7\8\9\7a";
    content: "#{x}\0\1\2\3\4\5\6\7\8\9\7a";
    content: "x#{\0\1\2\3\4\5\6\7\8\9}\7a";
    content: "x\a\A\b\B\c\C\d\D\e\E\f\F\g\G\7a";
    content: "#{x}\a\A\b\B\c\C\d\D\e\E\f\F\g\G\7a";
    content: "x#{\a\A\b\B\c\C\d\D\e\E\f\F\g\G}\7a";
}

baz {
    // another regression
    content: "xy#{\7a}";
}

@mgreter mgreter force-pushed the bugfix/issue_1115 branch 2 times, most recently from 737e571 to d928f7d Compare April 26, 2015 19:18
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 80.26% when pulling d928f7d on mgreter:bugfix/issue_1115 into daf9163 on sass:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 80.26% when pulling 8265b6b on mgreter:bugfix/issue_1115 into daf9163 on sass:master.

@mgreter mgreter changed the title Add hotfix for interpolation edge case Fix interpolation edge cases with escapes Apr 26, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 80.27% when pulling 7df70e6 on mgreter:bugfix/issue_1115 into daf9163 on sass:master.

mgreter added a commit that referenced this pull request Apr 26, 2015
Fix interpolation edge cases with escapes
@mgreter mgreter merged commit 89338a7 into sass:master Apr 26, 2015
@mgreter mgreter deleted the bugfix/issue_1115 branch July 28, 2015 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

variable interpolation causes additional string escaping
3 participants