Parent selector #959

ekskimn · 2015-03-17T06:48:00Z

Conflicts: ast.hpp

…elector * commit 'dea27e5e3cc61a0d9f00ffa3f598a1080c6502d6': (32 commits) Add plugin.cpp example to contrib directory Fix small issue with relative path creation on windows Fix small formatting error in Readme.md Fix issue with mingw32 build Fix some comments in C-API Move rpm spec file into contrib folder Remove code left overs from debugging Fix regression introduced in "root parent selector error" change sass_atof to be thread safe Add local unaware atof function Improve sourcemap output for prepended texts Implement number prefix parsing (/[\+\-]+/) Add error for parent selectors in root blocks Fix interpolation in media queries Improve comment parsing Add another fix for media query boundaries Parse line comments in blocks Fix illegal extending when no blocks are found Fix illegal extending accross media blocks More improvements ... Conflicts: ast.hpp

It handles all the unit tests except the interpolation one. Still working on that but sending the PR so you all can take a look

xzyfer · 2015-03-17T06:50:03Z

You can't understand my excitement to see movement on this @ekskimn. I'll dig into it soon.

This may be of interest to you @1xProgrammer.

coveralls · 2015-03-17T06:54:30Z

Coverage decreased (-0.09%) to 80.63% when pulling ef73c1b on ekskimn:parent-selector into dea27e5 on sass:master.

ekskimn · 2015-03-17T07:01:59Z

I know who that might be actually. I can coordinate with him.

On Mon, Mar 16, 2015 at 11:50 PM, Michael Mifsud notifications@github.com
wrote:

You can't understand my excitement to see movement on this @ekskimn
https://github.com/ekskimn. I'll dig into it soon.

This may be of interest to you @1xProgrammer
https://github.com/1xProgrammer.

—
Reply to this email directly or view it on GitHub
#959 (comment).

ekskimn · 2015-03-17T07:06:41Z

also, how concerned should I be that the coverage decreased? while i did
refactor some code around, I didn't actually create a lot of new code.

Just to be clear, what I did was separate out the contextualize that used
eval from the parts that didn't. I made eval use contextualize as part of
its composition but then created a new class contextualize_eval which
inherits from contextualize since they would share the same code, but adds
the selector schema and attribute selector methods that require an eval.
Then I set the expander to use the contextualize_eval and eval members as
it did before. The last thing i wanted was to have a bunch of if
(expression_type == Expression::SELECTOR) 's lying around.

On Mon, Mar 16, 2015 at 11:50 PM, Michael Mifsud notifications@github.com
wrote:

You can't understand my excitement to see movement on this @ekskimn
https://github.com/ekskimn. I'll dig into it soon.

This may be of interest to you @1xProgrammer
https://github.com/1xProgrammer.

—
Reply to this email directly or view it on GitHub
#959 (comment).

xzyfer · 2015-03-17T07:09:09Z

how concerned should I be that the coverage decreased

This is almost certainly because you've added more code that is covered by our current active specs.

xzyfer · 2015-03-17T07:09:55Z

I don't have a whole bunch of time atm, it's conference season for me. I'll take a look over the weekend at the latest.

ekskimn · 2015-03-17T07:11:55Z

#548 I got the interpolation working~!

ekskimn · 2015-03-17T07:15:01Z

ah dammit...i didn't see that....running those tests now...cross your fingers~!

coveralls · 2015-03-17T07:15:29Z

Coverage decreased (-0.1%) to 80.62% when pulling 776e997 on ekskimn:parent-selector into dea27e5 on sass:master.

am11 · 2015-03-17T07:17:19Z

expand.cpp

@@ -204,10 +204,9 @@ namespace Sass {
  {
    String* old_p = d->property();
    String* new_p = static_cast<String*>(old_p->perform(eval->with(env, backtrace)));
-    Expression* value = d->value()->perform(eval->with(env, backtrace));
-
+    Selector* p = selector_stack.size() >= 1 ? 0 : selector_stack.back();


Is this unused?

ah, It isn't supposed to be. I was using it in the next line and forgot to put it back in. Committing in a moment...

Oh ok. TYT! 👍

Fixed~! good catch btw.

coveralls · 2015-03-17T07:30:04Z

Coverage decreased (-0.1%) to 80.62% when pulling 324b14f on ekskimn:parent-selector into dea27e5 on sass:master.

xzyfer · 2015-03-17T07:36:39Z

Some of the specs for this feature are segfaulting.
Some are only failing due to whitespace issues (:tada:).

These tests wont run in CI because they're disabled.

You can run them locally checking out sass-spec and running the following

ruby sass-spec.rb -c /path/to/sassc -s spec/libsass-todo-tests/selectors

am11 · 2015-03-17T07:37:32Z

Please add contextualize_eval.cpp before this line: libsass.vcxproj#L167 and contextualize_eval.hpp here: libsass.vcxproj#L211.

xzyfer · 2015-03-17T07:38:54Z

inspect.cpp

+    if (p->selector()) {
+      p->selector()->perform(this);
+      string str = this->get_buffer();
+      str += ";";


Any reason not use?

append_string(";");

no particular reason. I can change that

ekskimn · 2015-03-17T07:44:29Z

looks like the seg faulting tests are mixin argument tests....let me see if i can whip that out....

xzyfer · 2015-03-17T07:45:43Z

Apologies for the nitpicks.

IMHO conventions are hugely important, more-so in open source code bases where people have limited time to contribute and want to know "the right way" to do things.

edits responding to comments for append_string typo for the broken line comma added contextualize_eval to the visual studio project file.

xzyfer · 2015-03-17T07:47:32Z

I'm off for now. I'll take a closer look at the contextualize changes ASAP.

ekskimn · 2015-03-17T07:47:46Z

no, i totally agree. I'm the same way with my dev team (ask them...they'll tell you i'm a dictator =) ) so I totally appreciate all the feedback

coveralls · 2015-03-17T07:49:53Z

Coverage decreased (-0.11%) to 80.61% when pulling 1c15df0 on ekskimn:parent-selector into dea27e5 on sass:master.

ekskimn · 2015-03-17T07:51:14Z

@am11 added those files to the vs project file.

mgreter · 2015-03-17T08:55:49Z

inspect.cpp

+      append_string(";");
+    }
+    else {
+      append_string("&");


If there should be an optional space, be sure to call append_optional_space() too!

coveralls · 2015-03-17T09:26:15Z

Coverage decreased (-0.08%) to 80.63% when pulling 92e3f0b on ekskimn:parent-selector into dea27e5 on sass:master.

xzyfer · 2015-03-17T09:31:04Z

contextualize_eval.cpp

+  Contextualize_Eval::~Contextualize_Eval() { }
+
+  Selector* Contextualize_Eval::fallback_impl(AST_Node* n)
+  { return Contextualize::fallback_impl(n); }


Sorry I think I'm still confused by this class.

Contextualize_Eval::operator()(Selector_List* s) { // Is there a difference between these two I'm missing? return Contextualize::operator ()(s); return s->preform(&instance_of_contextualize); }

If not then you can remove these operators and fallback to the Contextualize

Selector* Contextualize_Eval::fallback_impl(AST_Node* n) { return n->perform(&instance_of_contextualize); }

Which will hit Contextualize::fallback_impl if that operator type isn't defined.

Is it simply that you wish to opt-out of some Contextualize operators? It's unclear from this diff alone.

I think i see what you're saying. I think I was thinking of it as that this class is a sort of decorator or just extension on the original contextualize class. I started down the path you are suggesting and went with inheritance in this case because I felt that it felt more natural. I can try it the other way again

So i tried that and a lot more tests broke. it's 2:48 am here =) so i will need to sleep...but I welcome more feedback. I'm looking at that one error statement that I removed. The test was the closed issue 930 that was breaking when it is uncommented in. That test, though, passes, when commented out (not exactly a litmus test but just saying).

Cool, great work. I'll dig into it a bit and see what I come up with. If nothing else I'll better understand the intention here.

coveralls · 2015-03-17T09:36:40Z

Coverage decreased (-0.1%) to 80.62% when pulling 98977d8 on ekskimn:parent-selector into dea27e5 on sass:master.

xzyfer · 2015-03-17T12:33:41Z

~~Apply this patch get all specs baring the mixin-argument passing (minus the debug include)~~

Updated below

xzyfer · 2015-03-17T14:51:11Z

I got the test passing with this patch. It needs some cleaning up but it's 2am here.

Then I'd feel pretty comfortable getting this shipped if you

apply my patch
squash commits
do some tidy up

/cc @hcatlin (this may interest you)

coveralls · 2015-03-17T20:30:12Z

Coverage decreased (-0.4%) to 80.32% when pulling 5b6d06a on ekskimn:parent-selector into dea27e5 on sass:master.

coveralls · 2015-03-17T20:30:12Z

Coverage decreased (-0.4%) to 80.32% when pulling 5b6d06a on ekskimn:parent-selector into dea27e5 on sass:master.

xzyfer · 2015-03-17T23:41:52Z

@ekskimn can you please squash this commits.

AFAIK I see the outstanding todos:

listize to handle the rest of the selector nodes
determine if we can simplify the contextualizers.

am11 · 2015-03-17T23:57:59Z

can you please squash this commits.

@ekskimn,

One way to go about it is a two-fold (long) solution:

cd into/libsass
git remote add libsass https://github.com/sass/libsass
git pull --rebase libsass master
git pull --rebase
git push -f  # to rewrite the history

It will squash away those merge commits. Then:

git rebase -i HEAD~12    # here 12 is the number of commits you end-up with in this PR.
# editor will open, replace (only) `pick` with `f` for all commits but the first one.
# save and quit editor
git push -f   # to rewrite the history again

Following the exact steps will yield one clean commit. (however, you may want to keep them distributed)

xzyfer · 2015-03-18T00:57:39Z

I tried a couple strategies for squashing this branch but the merge commits caused too many conflicts.

For the sake of simplicity I've created a fully squashed patched rebased off the latest Libsass master.

cd libsass

# Sync you master with the latest Libsass master
git remote add libsass https://github.com/sass/libsass
git pull --rebase libsass master
git pull --rebase
git push -f  # to rewrite the history

# Unstage your changes and create a new commit
git checkout parent-selector
git reset --hard master
git clean -fd
git apply /path/to/complete.patch
git add .
git commit -m "Support parent selector & in values / and/or interpolations"

# rewrite history
git push -f parent-selector

ekskimn · 2015-03-18T09:56:34Z

Hey all. I am sorry i missed all these messages! I am in the middle of
moving (bought a house in silicon valley) and i had tried to squash the
commits earlier today and also got a lot of merge conflicts so i didnt
commit it.

If u need me to try again i can do so within the hour
2015. 3. 17. 오후 5:57에 "Michael Mifsud" notifications@github.com님이 작성:

I tried a couple strategies for squashing the branch but the merge commit
caused too many conflicts.

For the sake of simplicity I've created a fully squashed patched
https://www.dropbox.com/s/gndtujidx0bktz7/complete.patch?dl=0 rebased
off the latest Libsass master.

cd libsass

Sync you master with the latest Libsass master

git remote add libsass https://github.com/sass/libsass
git pull --rebase libsass master
git pull --rebase
git push -f # to rewrite the history

Unstage your changes and create a new commit

git checkout parent-selector
git reset --hard master
git clean -fd
git apply /path/to/complete.patch
git add .
git commit -m "Support parent selector & in values / and/or interpolations"

rewrite history

git push -f parent-selector

—
Reply to this email directly or view it on GitHub
#959 (comment).

xzyfer · 2015-03-18T10:02:01Z

Yeah the merge commits really mad a mess of the branch. To be honest I'd just create a new branch and apply the patch from my previous comment.

Make sure you sync you master with the latest Libsass master first.

git remote add upstream https://github.com/sass/libsass
git fetch upstream
git merge upstream/master
git push

xzyfer · 2015-03-18T10:02:33Z

Also, congrats on the new place.

ekskimn · 2015-03-18T10:04:09Z

Sure np. Give me an hour
2015. 3. 18. 오전 3:02에 "Michael Mifsud" notifications@github.com님이 작성:

Yeah the merge commits really mad a mess of the branch. To be honest I'd
just create a new branch and apply the patch from my previous comment.

Make sure you sync you master with the latest Libsass master first.

git remote add upstream https://github.com/sass/libsass
git fetch upstream
git merge upstream/master
git push

—
Reply to this email directly or view it on GitHub
#959 (comment).

ekskimn · 2015-03-18T10:59:39Z

PR is up there from a new branch, #965

xzyfer · 2015-03-18T11:11:47Z

Superseded by #965

Eric Kimn added 5 commits February 5, 2015 05:15

Support parent selector & in values / and/or interpolations sass#548

158a5af

Merge branch 'master' into parent-selector

d10b572

Conflicts: ast.hpp

merge from master to parent-selector

829fef7

issue 548

ef73c1b

It handles all the unit tests except the interpolation one. Still working on that but sending the PR so you all can take a look

sass#548 Fixed the interpolation~!

776e997

am11 reviewed Mar 17, 2015
View reviewed changes

sass#548 Fixing orphaned variable

324b14f

xzyfer reviewed Mar 17, 2015
View reviewed changes

sass#548 fixes for eval calling var->perform(this)

1c15df0

edits responding to comments for append_string typo for the broken line comma added contextualize_eval to the visual studio project file.

mgreter reviewed Mar 17, 2015
View reviewed changes

sass#548 removing commented out code thanks @mgreter.

92e3f0b

xzyfer reviewed Mar 17, 2015
View reviewed changes

sass#548 removed commented code

98977d8

sass#548 applied @xzyfer patch. Some clean up of commented out code

5b6d06a

xzyfer closed this Mar 18, 2015

xzyfer mentioned this pull request Mar 18, 2015

Implement support for & in SassScript #966

Merged

ekskimn deleted the parent-selector branch April 6, 2015 19:08

Parent selector #959

Parent selector #959

Conversation

ekskimn commented Mar 17, 2015

xzyfer commented Mar 17, 2015

coveralls commented Mar 17, 2015

ekskimn commented Mar 17, 2015

ekskimn commented Mar 17, 2015

xzyfer commented Mar 17, 2015

xzyfer commented Mar 17, 2015

ekskimn commented Mar 17, 2015

ekskimn commented Mar 17, 2015

coveralls commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 17, 2015

xzyfer commented Mar 17, 2015

am11 commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekskimn commented Mar 17, 2015

xzyfer commented Mar 17, 2015

xzyfer commented Mar 17, 2015

ekskimn commented Mar 17, 2015

coveralls commented Mar 17, 2015

ekskimn commented Mar 17, 2015

Choose a reason for hiding this comment

coveralls commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 17, 2015

xzyfer commented Mar 17, 2015

xzyfer commented Mar 17, 2015

coveralls commented Mar 17, 2015

coveralls commented Mar 17, 2015

xzyfer commented Mar 17, 2015

am11 commented Mar 17, 2015

xzyfer commented Mar 18, 2015

ekskimn commented Mar 18, 2015

Sync you master with the latest Libsass master

Unstage your changes and create a new commit

rewrite history

xzyfer commented Mar 18, 2015

xzyfer commented Mar 18, 2015

ekskimn commented Mar 18, 2015

ekskimn commented Mar 18, 2015

xzyfer commented Mar 18, 2015