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

Refactor contextualize into eval [WIP] #1217

Closed
wants to merge 9 commits into from

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented May 15, 2015

Remove contextualize and contextualize_eval by placing the functionallity into eval. Also refactors usage of Parent_Selector and gets rid of Selector_Reference. We now pretty much only store state on the expander itself (selector_stack, env_stack, etc). First iteration that should not break CI.

@mgreter mgreter self-assigned this May 15, 2015
@mgreter mgreter force-pushed the feature/refactor-contextualize branch from 938d699 to e5e003d Compare May 16, 2015 11:26
@mgreter mgreter force-pushed the feature/refactor-contextualize branch from e5e003d to 9189202 Compare May 17, 2015 13:29
@xzyfer
Copy link
Contributor

xzyfer commented May 18, 2015

Needs a clean up but generally speaking it looks sane

@mgreter mgreter force-pushed the feature/refactor-contextualize branch from 7d12745 to dd7e37e Compare May 18, 2015 03:19
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 80.22% when pulling 8671263 on mgreter:feature/refactor-contextualize into 221d7fb on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented May 19, 2015

Does your parent selector refactor help with #1210 by any chance?

@mgreter
Copy link
Contributor Author

mgreter commented May 19, 2015

Well, it should help to make more sense out of the actual code. I actually did not understand why it was doing what it was doing. So yes, it could help with that, since it is more obvious where and which parent selector we are referring to IMHO. But it really depends on the underlying problem :)

@mgreter mgreter added this to the 3.3 milestone May 19, 2015
@mgreter
Copy link
Contributor Author

mgreter commented May 19, 2015

Took a look at your linked issue and I guess I know how we can fix it. IMHO we need another selector type beside Parent_Selector, namely Root_Selector. Currently I thought I could just use NULL to represent that, but this would need quite some refactoring, since we have a lot of places where we would need to check for that potential NULL pointer. Parent selectors now only "live" in the selector_stack, so you can more easily tamper with it. I used this technique to implement Parent_Selector rendering (pop one selector off the stack, render recursively and push it back once finished). So in theory we just should need to add a Root_Selector to that stack when rendering/expanding root nodes. At least that's what I would guess. I'm currently focusing in another area, which will take quite some time (values/nodes and memory management, plus some basic performance considerations; currently catching up with the latest tricks in mark and sweep GC implementations). So I have absolutely no ETA if/when I will come around to implement this, so I thought I document it here for future references.

@xzyfer
Copy link
Contributor

xzyfer commented May 31, 2015

Can this be applied without #1249?

It looks like #1249 does a bunch of this work as well.

@mgreter mgreter closed this Jun 13, 2015
@mgreter mgreter deleted the feature/refactor-contextualize branch July 28, 2015 10:06
xzyfer added a commit to xzyfer/sass-spec that referenced this pull request Oct 26, 2015
xzyfer added a commit to xzyfer/libsass that referenced this pull request Oct 26, 2015
Ruby Sass explicitly unquotes the results of interpolations in
selectors. This patch is very thorough but it address all the
use cases I think of and it fixes the regression causing people
issues.

Fixes sass#1217
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.

3 participants