Refactoring and base work for 3.3 #1249
Conversation
|
Looked at the cssize changes, they're fine. |
mgreter
force-pushed
the
refactoring/parser-for-3.3
branch
from
c82290e
to
81a373b
Compare
mgreter
force-pushed
the
refactoring/parser-for-3.3
branch
from
006a63b
to
671e92c
Compare
|
Really liking the changes in this branch, everything is really making a lot more sense as someone less familiar with the code base! |
|
@mgreter if you can get this ready for review in a week I'm cool with
|
|
@onedayitwillmake Thanks a lot for your feedback! I didn't actually do much work in the parser/eval before, so I had the get myself familiar with it first too. I see some more work in pseudo selectors, since they trigger specific "is_superselector" behavior. But we may postpone this after an initial release. @xzyfer I'd say lets set a due date and I will see how much I can get in. I'm now at a stage where I think I have parent selectors covered quite well (took me about 3 days to get it right and to wrap my head around it). Unfortunately I will not have that much time over the next weekends. |
mgreter
force-pushed
the
refactoring/parser-for-3.3
branch
from
7411b6b
to
a71cfad
Compare
|
Made some major break thoughs by removing the re-parsing step in ruleset expander. Basically re-implemented the complete selector parsing, expanding and evaluating part. Big performance improvements in the loop and number evaluation code parts and also a lot of smaller performance improvements from parser, expand and eval refactoring. This branch should now pass the following open issues: #941, #948, #1029, #1043, #1061, #1170, #1207, #1210, #1214, #1219, #1227, #1230, #1240, #1253, #1255, #1257, #1258, #1260, #1263, #1269, #1273, #1281, #1283, #1285, #1291, #1297 Now also passes these
There is one minor white-space issue, that prevents
Example: @mixin dummy {
@keyframes {
@content;
}
}
@include dummy {
foo, bar { baz: test; }
}Ruby sass output (compressed!): @keyframes{foo, bar{baz:test}}Current libsass output (compressed!): @keyframes{foo,bar{baz:test}}IMHO libsass is correct here to remove the space after the comma separator. |
mgreter
force-pushed
the
refactoring/parser-for-3.3
branch
3 times, most recently
from
f53be75
to
133b20a
Compare
|
@xzyfer cleaned up the most obvious stuff but all in all I'm pretty much done for the first step. So this is pretty much open for review and comments now. I added a few crutches to solve a few edge cases for now. A complete solution for these edge cases will need some more work. These are namely:
Other than that I feel pretty confortable with the final result! ToDo: need to check if parser errors out correctly for missing scope brackets and semicolons ... |
mgreter
force-pushed
the
refactoring/parser-for-3.3
branch
from
133b20a
to
7ccfaca
Compare
mgreter
force-pushed
the
refactoring/parser-for-3.3
branch
2 times, most recently
from
a4fd8b0
to
a262eed
Compare
|
It's midnight here. I'll rerun this branch over our code base in the morning. If there are no other regressions I'll ship it! |
|
I've done some quick testing and I'm see regressions in newlines in selectors. A quick example Result master Ruby Sass Removing the Result master Ruby Sass |
|
Also seeing new lines in selectors appearing after combinators Result Master & Ruby Sass |
| @@ -899,6 +900,14 @@ namespace Sass { | |||
| Compound_Selector* head = c->head(); | |||
| Complex_Selector* tail = c->tail(); | |||
| Complex_Selector::Combinator comb = c->combinator(); | |||
|
|
|||
| if (c->has_line_feed()) { | |||
There was a problem hiding this comment.
This block appears to be the cause of the broken selector output reported in #1249 (comment) and #1249 (comment)
There was a problem hiding this comment.
I already got a fix for the first issue ... they don't seem related much ...
|
Another regression: %foo1 {
> bar > bam &:before,
> bar > bam &:after {
bar: baz;
}
}
test1 {
@extend %foo1;
}Result before the fix: > bar > bam &:before, > bar > bam &:after {
bar: baz; }Result after additional fix: > bar > bam test1:before,
> bar > bam test1:after {
bar: baz; }IMO the additional line-feed makes sense and improves readability (and is more what I would expect from the given input). So I vote to leave it that way and call it a ruby sass bug!? IMHO once everything is more stable it should be possible to simplify the code even more. There are quite a few duplicated code points. |
👍 I don't care much for matching Sass' whitespace semantics perfectly because they're too unpredictable. Like you say as long as the output is readable it's fine. |
|
If CI goes green 🚢 it 🎉 🎆 🍕 |
|
Waiting for AppVeyor then ... 🚀 🎸 |
Refactoring and base work for 3.3
|
Congrats ㊗️ |
Pushing my latest refactoring work to public since it is currently at a working state that solves all open
@at-rootissues, plus a few other open issues.PR is still too dirty too actually commit the work to master branch. It also introduced a minor whitespace (linefeed preserving) regressions (from issue 1208), since the fix we added for it doesn't seem to be 100% correct. I will try to tackle the linefeed preserving again at a later stage (the logic for this seems to be clear so far).The following issues should pass with this WIP branch:
&selector interpolation in@at-rootblocks (Error with&selector interpolation in@at-rootblocks #1043)is_superselectorfunctions (Implement selector functions #963)This branch also includes quite a few performance optimizations.
It brought my sass-spec test suite runtime from 27s down to 7s!
TODO
Improve function call return value handling(3f2daf3)Improve deprecation handling in unquote function(93d3328)Fix unquote handling for non string types(f43b881)Add code improvements for SUN OS compilation(b523c9e) to it's own PR ([Proposal] Add code improvements for SUN OS compilation #1326)Fix memory leaks and improve 'input_source' handling(af12e9b)input_sourcehandling #1327)Implement initial parts for reference combinators(2f07ba3)