Enable url function overloading #1010
Conversation
mgreter
force-pushed
the
bugfix/issue_674
branch
2 times, most recently
from
b802ecf to
cc1fde7
Compare
|
CI seems finally happy! IMO this is ready to be merged! |
|
Could you add a spec for this to prevent future regressions? |
| @@ -1118,10 +1130,10 @@ namespace Sass { | |||
| { | |||
| Expression* conj1 = parse_conjunction(); | |||
| // if it's a singleton, return it directly; don't wrap it | |||
| if (!peek< sequence< kwd_or, negate< identifier > > >()) return conj1; | |||
| if (!peek_css< kwd_or >()) return conj1; | |||
There was a problem hiding this comment.
Does peek_css force word boundaries? Otherwise this is a regression.
There was a problem hiding this comment.
No, but kwd_or is forced to have word boundaries 😉
|
@xzyfer should we include this in next 3.2.0-beta.5 release? |
|
I think this can wait. It's an edge case case. Now is probably a good time
|
|
I wanted to have the real feature freeze when the next node-sass beta is out. As I have written in #1020, we should create a new beta once node-sass is ready to release its next beta. IMO we can add a few more bugfixes until then. But from then on I would say we close the window and really only commit bugfixes. BTW. the original issue was marked as a bug, so technically this is a bugfix 😉 |
|
Lol technicalities. Ship it.
|
mgreter
force-pushed
the
bugfix/issue_674
branch
from
cc1fde7 to
fd1814c
Compare
1 similar comment
Enable url function overloading
@xzyfer maybe give it a review and we probably also want to include in 3.2.0? I don't really see any reason that tests shouldn't have catched any obvious regressions this could introduce!
And it does fix #674 (the refactor made the code actually a bit slimer)!