Support for @mixin, @include and @extend

- Added @mixin and @include support for top-level and declarations.
- Added parameter support for mixins.
- Support var definition with multiple parameters as an @include parameter.
- Support @extend and @extend with nested selectors.
- Removed hoisting var and cycle checks - can only reference known vars.
- Added bunch of tests.


BUG=

[Jennifer Messerly, 2013-09-18 06:44:15]

this is really cool! i just have minor comments. Added Nathan as he has more*
expertise about this kind of CSS transform.

(where more == infinitely more ;) )

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/css.dart
File pkg/csslib/lib/css.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/css.dart#ne...
pkg/csslib/lib/css.dart:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
2013?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart
File pkg/csslib/lib/parser.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:768: if (_peekIdentifier()) {
is identifier optional? it doesn't appear so from the grammar comment above

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:770: }
TODO here about parsing the args?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:780: var directive = processDirective();
is it possible to reuse any existing parser code here?

from the grammar snippet I was expecting something a bit simpler.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:809: newDecls.add(new
IncludeMixinAtDeclaration(include, include.span));
long line

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:819: declGroup.declarations.forEach((decl) {
I would probably just use a normal for-in loop here

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:833: if (decl is IncludeMixinAtDeclaration) {
this looks similar to:

   productions.add(decl is IncludeMixinAtDeclaration ? decl.include : decl);

... above, maybe use the same style in both places?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:855: // TODO(terry): TBD
issue warning about not implemented feature?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:859: IncludeDirective processInclude(Span span, [bool
eatSemiColon = true]) {
for bools it's kind of nice to use named arguments like:

{bool eatSemicolon: true}

... a tad more readable at the call site

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
File pkg/csslib/lib/src/analyzer.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:140: /*
remove?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
File pkg/csslib/test/mixin_test.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:10: var  options = ['--warnings_as_errors',
'--no-colors', 'memory'];
extra space here?

[nweiz, 2013-09-18 22:40:54]

I mention this several times in my individual comments, but the distinction
between "top-level" and "declaration" mixins is confusing, user-unfriendly, and
probably leads to more complicated code as well. A mixin should be able to
contain rules, CSS properties, includes, or directives. It should be able to
include all of these at once or any of them individually. There should also,
naturally, be tests for all of these things.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart
File pkg/csslib/lib/parser.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:449: processDirective() {
Why is this (as well as many other productions) public?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:759: /* Stylet grammar:
"Stylet" -> "Mixin"

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:762: *    ruleset | properties
This should be "[ruleset | property | directive]*", or if you want to be more
correct "[ruleset | property | directive] (';' [ruleset | property |
directive])*" with the caveat that the semicolon isn't necessary if the
preceding production has a brace-delimited block.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:783: _maybeEat(TokenKind.SEMICOLON);
Why are you using [_maybeEat] here? Any number of successive semicolons should
be allowed between children of a directive, not just one. Also, the following is
invalid (due to the lack of a semicolon between the two sub-directives), where
it looks like it would be allowed by this code: "@mixin foo {@some-directive
@another-directive}".

The algorithm that Sass uses for this sort of parsing looks like this:

    while consume(';') or last child has child nodes:
      last child <- process block child

where a block child could be empty.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:793: break;
Why are you breaking here? Mixins can contain more than one ruleset, or rulesets
mixed in with other types of children.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:803: })) {
Style nit: if you're going to write this if statement like this, it should be
indented as so:

    if (declGroup.declarations.any((decl) {
          return decl is Declaration && decl is! IncludeMixinAtDeclaration;
        })) {

The baseline indentation for a continued expression is +4 spaces, and the body
of the closure should be +2 spaces in addition to that.

That said, it's probably cleaner and more readable if you move the expression
into a variable rather than using it inline in the if statement.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:807: // this mixin is a declaration mixin not a
top-level mixin.
This isn't a distinction that should exist.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:818: // no a declaration group.
If I interpreted this sentence correctly, it should be "Declarations are just
@includes. Make it a list of productions, not a declaration group."

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:860: /* Stylet grammar:
"Stylet" -> "Include"

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:867: if (_peekIdentifier()) {
As in John's comment for @mixin, this shouldn't be optional.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:1491: //  declaration:  property ':' expr prio?
Shouldn't this include variables and now @include as well?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
File pkg/csslib/lib/src/analyzer.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:32: mixinDefs = new
MixinDefinitions(_styleSheets);
It's not correct to locate all mixin definitions before performing any
@includes. @mixin declarations aren't hoisted; they're only visible to styles
that come after them.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:37: _messages.warning("mixin cycle detected
@mixin ${cycle.def.name}",
"Mixin cycle detected for @mixin ${cycle.def.name}."

Shouldn't this produce an error rather than a warning if a mixin cycle is
detected? Also, shouldn't the error message list every mixin in the cycle, not
just one of them?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:39: // TODO(terry): What if no mixin definition
for a @include an error?
"What if there are no mixin definitions for an @include? An error?"

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:45: var varCycles = findAllVarCycles();
Variables should be eagerly evaluated, so cycle detection shouldn't be needed.
If a variable refers to a variable that hasn't yet been assigned, that should
just produce a reference error.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:61: ..visitStyleSheet(styleSheet));
Running separate visitors for all of these seems inefficient and less clear than
doing all the transforms in one pass. As more features get added, are you
planning to add a separate visitor with its own pass over the AST for each of
them?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:63: // Expand  @include in declarations.
Nit: extra space before "@include".

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:68: // Any missing @mixins only check if
everything is good up to this point.
I don't understand what this comment means.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:77: new
RemoveMixinsAndIncludes()..visitStyleSheet(styleSheet));
Style nit: indent +2 spaces.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:89: mixinDefs.map.values.forEach((value) {
As John's mentioned elsewhere, use a for loop rather than [forEach] where
possible.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:104: * [includes] list of IncludeDirective or
IncludeMixinAtDeclaration.
If you're going to include a doc comment, it shouldn't just document a single
parameter.

Also, use complete sentences. "[includes] is a list of IncludeDirectives or
IncludeMixinAtDeclarations."

It's unclear to me what the difference is between these two constructs.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:109: if (visited == null) visited = new Set();
Explain the function of [visited] and how it differs from [visiting].

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:633: final Map<String, MixinDef> map = new
Map<String, MixinDef>();
This type declaration is unnecessary. The type is obvious from the value you're
assigning.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:710: static bool replace(StyleSheet styleSheet,
IncludeDirective include,
You have three different ways of invoking a visitor in this file. For
[MixinDefinitions], you just construct it and it calls [visitStyleSheet]
internally. For [ExpandTopLevelIncludes], you construct it and then call
[visitStyleSheet] manually. And then for this class, you have a static method
that handles the construction and calling of [visitStyleSheet]. Make these
consistent with one another.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:714: return visitor._foundAndReplaced;
This value is never used.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:730: var index = node.rulesets.indexOf(_include
as dynamic);
What is "as dynamic" doing here?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:733: node.rulesets.replaceRange(index, index +
1, [new NoOp()]);
Why aren't you replacing the include with _newRules? It seems very weird to
replace it with a noop and then add the new rules afterwards.

That said, I still think it would be cleaner to use a single visitor for all
these replacements, and to have a more generic way for that visitor to replace a
node -- or better, non-destructively construct a new AST with different nodes.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:752: if
(defs.map.containsKey(node.include.name)) {
Short-circuit if this is false to avoid extra nesting.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:759: // we'll convert to a list of
IncludeMixinAtDeclaration(s).
It doesn't make sense to say "We're X. If so, ...".

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:760: var orgRulesets = mixinDef.rulesets;
What does "org" mean?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:787: * Look for the [ruleSet] inside of an
@media directive; if found then replace
"a @media"

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:794: return visitor._foundAndReplaced;
This is also never used.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:807: if (decl.include == _include) {
Use "&&" rather than nested if statements.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:813: }
This i++/for/break stuff is really complicated and completely obscures your
intention. Add a utility function [indexWhere] that takes a list and a function
and returns the first index of an element in the iterable for which that
function returns true.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:815: if (index != -1) {
Use null rather than -1 as a flag value.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:834: _messages.warning("@mixin ${node.name} is
undefined.", node.span);
This should be an error.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:844: class RemoveMixinsAndIncludes extends
Visitor {
Don't the other visitors remove the mixins and includes? Why are you adding
noops if they're just going to be removed?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:847: bool _nodesToRemove(node) =>
This name sounds like a variable, not a function. Maybe something like
[_shouldRemoveNode].

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:852: while (--index >= 0) {
It's never clearer to use "--variable" in an expression than to manually expand
it. Don't force your reader to remember the distinction between "x++" and "++x".

This particular case would also be much clearer if you just used
[List.removeWhere].

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
File pkg/csslib/lib/src/css_printer.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
pkg/csslib/lib/src/css_printer.dart:10: class CssPrinter extends Visitor {
If this is printing the CSS tree, why does it need to print mixins? If it's
printing a string representation of the SCSS tree, the name and documentation
should indicate that.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
pkg/csslib/lib/src/css_printer.dart:209: if (topLevel) emit(_newLine);    //
Emit newline for top-level @include.
Unnecessary spaces before the comment.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
pkg/csslib/lib/src/css_printer.dart:211: if (topLevel) emit(';');
Why wouldn't non-top-level @includes have a semicolon?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
pkg/csslib/lib/src/css_printer.dart:214: void
visitContentDirective(ContentDirective node) {
If you don't support content directives yet, just don't include the code.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/tree.dart
File pkg/csslib/lib/src/tree.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/tree.da...
pkg/csslib/lib/src/tree.dart:506: final bool varArgs;
As with @content, don't include infrastructure for features that aren't
supported. You can add these once you actually support arguments.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/tree.da...
pkg/csslib/lib/src/tree.dart:514: /** To support defining a SASS @mixin. */
Only the first letter of "Sass" is capitalized.

These documentation comments are way too short. If I'm reading this as someone
who doesn't know what a mixin is, I'm completely befuddled. Actually explain
what a mixin is. Use complete sentences. If you have multiple classes for
similar things -- IncludeDirective vs IncludeMixinAtDeclaration, for example --
explain the difference.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
File pkg/csslib/test/mixin_test.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:10: var  options = ['--warnings_as_errors',
'--no-colors', 'memory'];
"final", not "var".

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:12: void topLevelMixin() {
Why are you defining a separate function for each test? pkg/unittest provides a
pre-existing way of dividing up and naming test cases; you should use that.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:14: final input1 = r'''
Why is this [input1]? There's no other [input] variables in this scope. Same
goes for [generated1] and [stylesheet1].

This should also be "var", not "final", since it's a local variable.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:23: ''';
These no-indentation strings are difficult to read, especially when [generated]
starts on the same line as the string opener. One thing Bob and I have done in
the past is do a small amount of pre-processing to allow the input and output to
be indented eight spaces in the source of the test.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:30: final stylesheet1 = compileCss(input1,
errors: errors, opts: options);
Using an out parameter like [errors] here is a code smell. Why isn't
[compileCss] just throwing an exception?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:32: expect(stylesheet1 != null, true);
This could be written as "expect(stylesheet, isNotNull)". However, I don't think
it makes sense to make this assertion at all. The feature you're testing --
@mixin -- has nothing to do with whether the stylesheet that's returned is null.
If it does happen to be null for some unexpected reason, other asserts will
catch that.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:33: expect(errors.isEmpty, true, reason:
errors.toString());
expect(errors, isEmpty, ...)

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:36: // TODO(terry): Test using a declaration
@mixin as a top-level @include.
Why is this a TODO and not an actual test?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:74: void topLevelMixinMultiRulesets() {
What do multiple rulesets have to do with this test? It seems like the last test
tests multiple rulesets. This looks like it's testing transitive mixin
inclusion. If that's the case, it should get rid of "@mixin a", as that's not
contributing to the behavior being tested.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:120: void topLevelMixinMultiRulesets2() {
I have no idea what this test is testing. Is it just... a lot of mixins? Why
would six mixins fail when two mixins didn't? This test seems superfluous.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:193: void topLevelMixinSelectors() {
All the previous tests had selectors. Saying that this one tests selectors
doesn't tell me anything. Maybe this should be titled "test top-level mixin with
multiple selectors"?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:275: void declMixinNestedIncludes() {
As above, "div-border" is unrelated to what this test is testing.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:312: void declMixinNestedIncludes2() {
I can't understand what this test tests that the previous test didn't.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:354: @mixin div-inside(@dist) {
If your arguments are going to use this syntax, you're not implementing SCSS,
you're writing your own idiosyncratic stylesheet language. That's fine if you
don't mind sacrificing user knowledge and cross-compatibility, but if you go
that route, you can't refer to them as "Sass mixins" or use ".scss" as the file
extension.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:549: void badDeclarationInclude() {
This shouldn't cause an error.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:577: void badTopInclude() {
This *should* cause an error.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:622: expect(errors.isNotEmpty, true, reason:
errors.toString());
expect(errors, isNotEmpty)

[reason] here doesn't do anything, since it'll only be used when errors is empty
anyway.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:625: expect(error.message, 'mixin cycle detected
@mixin a');
Test the type of the error, not the message. Testing the message couples the
test to the specific error text used.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:662: test('top-level mixin 2 @includes',
topLevelMixin2Includes);
"2" -> "two", here and elsewhere. Also, at least use sentence fragments for test
descriptions (e.g. "top-level mixin with two @includes").

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:670: test('declaration mixin includes #2',
declMixinNestedIncludes2);
Adding "#2" here doesn't tell me anything about the difference between the two
tests.  Be more descriptive in your test descriptions.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:677: // TODO(Terry): More tests to enable.
Don't include tests that don't work. You can add these later along with the
feature that makes them work. This is what source control is for.

[terry, 2013-10-09 03:40:33]

Added parameters, removed hoisting of vars and mixins, support for @extend,
added more tests.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/css.dart
File pkg/csslib/lib/css.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/css.dart#ne...
pkg/csslib/lib/css.dart:1: // Copyright (c) 2012, the Dart project authors. 
Please see the AUTHORS file
Changed to 2013.  I thought the copyright was the date of the file creation do
we rev whenever we change the file, or major file entry points has the current
year, or all files should be rev at beginning of new year?

On 2013/09/18 06:44:15, John Messerly wrote:
> 2013?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart
File pkg/csslib/lib/parser.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:449: processDirective() {
The parser should be a hidden class (_Parser).
On 2013/09/18 22:40:54, nweiz wrote:
> Why is this (as well as many other productions) public?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:759: /* Stylet grammar:
On 2013/09/18 22:40:54, nweiz wrote:
> "Stylet" -> "Mixin"

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:762: *    ruleset | properties
On 2013/09/18 22:40:54, nweiz wrote:
> This should be "[ruleset | property | directive]*", or if you want to be more
> correct "[ruleset | property | directive] (';' [ruleset | property |
> directive])*" with the caveat that the semicolon isn't necessary if the
> preceding production has a brace-delimited block.

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:768: if (_peekIdentifier()) {
It's not optional, it's required - changed.

On 2013/09/18 06:44:15, John Messerly wrote:
> is identifier optional? it doesn't appear so from the grammar comment above

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:770: }
On 2013/09/18 06:44:15, John Messerly wrote:
> TODO here about parsing the args?

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:780: var directive = processDirective();
Was able to simplify a bit but this is really handling special cases for mixins
both declarations and top-level stuff which are very different.  The parsing of
the decl vs ruleset is of course re-used code.

On 2013/09/18 06:44:15, John Messerly wrote:
> is it possible to reuse any existing parser code here?
> 
> from the grammar snippet I was expecting something a bit simpler.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:783: _maybeEat(TokenKind.SEMICOLON);
Okay, I fixed it.

On 2013/09/18 22:40:54, nweiz wrote:
> Why are you using [_maybeEat] here? Any number of successive semicolons should
> be allowed between children of a directive, not just one. Also, the following
is
> invalid (due to the lack of a semicolon between the two sub-directives), where
> it looks like it would be allowed by this code: "@mixin foo {@some-directive
> @another-directive}".
> 
> The algorithm that Sass uses for this sort of parsing looks like this:
> 
>     while consume(';') or last child has child nodes:
>       last child <- process block child
> 
> where a block child could be empty.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:793: break;
Was able to eliminate the whole nestedSelector chunk of code - no longer
relevant (plus not used anyway).
On 2013/09/18 22:40:54, nweiz wrote:
> Why are you breaking here? Mixins can contain more than one ruleset, or
rulesets
> mixed in with other types of children.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:803: })) {
Ok, changed the indentation.
On 2013/09/18 22:40:54, nweiz wrote:
> Style nit: if you're going to write this if statement like this, it should be
> indented as so:
> 
>     if (declGroup.declarations.any((decl) {
>           return decl is Declaration && decl is! IncludeMixinAtDeclaration;
>         })) {
> 
> The baseline indentation for a continued expression is +4 spaces, and the body
> of the closure should be +2 spaces in addition to that.
> 
> That said, it's probably cleaner and more readable if you move the expression
> into a variable rather than using it inline in the if statement.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:807: // this mixin is a declaration mixin not a
top-level mixin.
Warnings are returned if a mixin has both decls and rulesets e.g.,

@mixin a {
  foo { color: red; }
  background-color: blue;
}

@include a;

gives a warning instead of magically losing the background-color.

On 2013/09/18 22:40:54, nweiz wrote:
> This isn't a distinction that should exist.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:809: newDecls.add(new
IncludeMixinAtDeclaration(include, include.span));
On 2013/09/18 06:44:15, John Messerly wrote:
> long line

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:818: // no a declaration group.
On 2013/09/18 22:40:54, nweiz wrote:
> If I interpreted this sentence correctly, it should be "Declarations are just
> @includes. Make it a list of productions, not a declaration group."

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:819: declGroup.declarations.forEach((decl) {
On 2013/09/18 06:44:15, John Messerly wrote:
> I would probably just use a normal for-in loop here

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:833: if (decl is IncludeMixinAtDeclaration) {
On 2013/09/18 06:44:15, John Messerly wrote:
> this looks similar to:
> 
>    productions.add(decl is IncludeMixinAtDeclaration ? decl.include : decl);
> 
> ... above, maybe use the same style in both places?

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:855: // TODO(terry): TBD
On 2013/09/18 06:44:15, John Messerly wrote:
> issue warning about not implemented feature?

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:859: IncludeDirective processInclude(Span span, [bool
eatSemiColon = true]) {
On 2013/09/18 06:44:15, John Messerly wrote:
> for bools it's kind of nice to use named arguments like:
> 
> {bool eatSemicolon: true}
> 
> ... a tad more readable at the call site

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:860: /* Stylet grammar:
On 2013/09/18 22:40:54, nweiz wrote:
> "Stylet" -> "Include"

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:867: if (_peekIdentifier()) {
Right fixed and handled parameters too.
On 2013/09/18 22:40:54, nweiz wrote:
> As in John's comment for @mixin, this shouldn't be optional.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/parser.dart...
pkg/csslib/lib/parser.dart:1491: //  declaration:  property ':' expr prio?
It does.  See VAR_DEFINITION and DIRECTIVE_INCLUDE in this function.

On 2013/09/18 22:40:54, nweiz wrote:
> Shouldn't this include variables and now @include as well?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
File pkg/csslib/lib/src/analyzer.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:32: mixinDefs = new
MixinDefinitions(_styleSheets);
Yeah, I liked the idea of hoisting like a class but it's not SASS so I've undone
this.
On 2013/09/18 22:40:54, nweiz wrote:
> It's not correct to locate all mixin definitions before performing any
> @includes. @mixin declarations aren't hoisted; they're only visible to styles
> that come after them.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:37: _messages.warning("mixin cycle detected
@mixin ${cycle.def.name}",
Without hoisting cycle detection is removed.
On 2013/09/18 22:40:54, nweiz wrote:
> "Mixin cycle detected for @mixin ${cycle.def.name}."
> 
> Shouldn't this produce an error rather than a warning if a mixin cycle is
> detected? Also, shouldn't the error message list every mixin in the cycle, not
> just one of them?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:45: var varCycles = findAllVarCycles();
Yes also removed variable hoisting.
On 2013/09/18 22:40:54, nweiz wrote:
> Variables should be eagerly evaluated, so cycle detection shouldn't be needed.
> If a variable refers to a variable that hasn't yet been assigned, that should
> just produce a reference error.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:61: ..visitStyleSheet(styleSheet));
Yes, for now that is the mode we've taken in the dwc and csslib for ease of
disabling and enabling a feature.  We plan on combining once we have a need from
a perf point of view.  But it's a good idea to add a TODO so people realize why
we've done this.

On 2013/09/18 22:40:54, nweiz wrote:
> Running separate visitors for all of these seems inefficient and less clear
than
> doing all the transforms in one pass. As more features get added, are you
> planning to add a separate visitor with its own pass over the AST for each of
> them?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:63: // Expand  @include in declarations.
On 2013/09/18 22:40:54, nweiz wrote:
> Nit: extra space before "@include".

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:68: // Any missing @mixins only check if
everything is good up to this point.
Functionality removed when hoisting of mixin removed - was for references to
forward undefined mixins.
On 2013/09/18 22:40:54, nweiz wrote:
> I don't understand what this comment means.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:77: new
RemoveMixinsAndIncludes()..visitStyleSheet(styleSheet));
Code removed.
On 2013/09/18 22:40:54, nweiz wrote:
> Style nit: indent +2 spaces.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:89: mixinDefs.map.values.forEach((value) {
Cycle code removed.
On 2013/09/18 22:40:54, nweiz wrote:
> As John's mentioned elsewhere, use a for loop rather than [forEach] where
> possible.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:104: * [includes] list of IncludeDirective or
IncludeMixinAtDeclaration.
Cycle code removed.
On 2013/09/18 22:40:54, nweiz wrote:
> If you're going to include a doc comment, it shouldn't just document a single
> parameter.
> 
> Also, use complete sentences. "[includes] is a list of IncludeDirectives or
> IncludeMixinAtDeclarations."
> 
> It's unclear to me what the difference is between these two constructs.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:109: if (visited == null) visited = new Set();
cycle code removed.
On 2013/09/18 22:40:54, nweiz wrote:
> Explain the function of [visited] and how it differs from [visiting].

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:140: /*
entire function removed.
On 2013/09/18 06:44:15, John Messerly wrote:
> remove?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:633: final Map<String, MixinDef> map = new
Map<String, MixinDef>();
Yep but editor code completion didn't work w/o it but will remove.
On 2013/09/18 22:40:54, nweiz wrote:
> This type declaration is unnecessary. The type is obvious from the value
you're
> assigning.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:710: static bool replace(StyleSheet styleSheet,
IncludeDirective include,
Function was re-written this no longer exist.
On 2013/09/18 22:40:54, nweiz wrote:
> You have three different ways of invoking a visitor in this file. For
> [MixinDefinitions], you just construct it and it calls [visitStyleSheet]
> internally. For [ExpandTopLevelIncludes], you construct it and then call
> [visitStyleSheet] manually. And then for this class, you have a static method
> that handles the construction and calling of [visitStyleSheet]. Make these
> consistent with one another.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:714: return visitor._foundAndReplaced;
function re-written not an issue now.
On 2013/09/18 22:40:54, nweiz wrote:
> This value is never used.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:730: var index = node.rulesets.indexOf(_include
as dynamic);
_include can be different either declarative vs/ ruleset @include.  Should
consider a better AST node to fix this issue.
On 2013/09/18 22:40:54, nweiz wrote:
> What is "as dynamic" doing here?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:733: node.rulesets.replaceRange(index, index +
1, [new NoOp()]);
Only the resolve the @include once.

On 2013/09/18 22:40:54, nweiz wrote:
> Why aren't you replacing the include with _newRules? It seems very weird to
> replace it with a noop and then add the new rules afterwards.
> 
> That said, I still think it would be cleaner to use a single visitor for all
> these replacements, and to have a more generic way for that visitor to replace
a
> node -- or better, non-destructively construct a new AST with different nodes.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:752: if
(defs.map.containsKey(node.include.name)) {
Really, seems clunky to call super for the short-circuit and normal path...
On 2013/09/18 22:40:54, nweiz wrote:
> Short-circuit if this is false to avoid extra nesting.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:759: // we'll convert to a list of
IncludeMixinAtDeclaration(s).
yep, changed.
On 2013/09/18 22:40:54, nweiz wrote:
> It doesn't make sense to say "We're X. If so, ...".

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:760: var orgRulesets = mixinDef.rulesets;
original - missing 'i'  origRulesets
On 2013/09/18 22:40:54, nweiz wrote:
> What does "org" mean?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:787: * Look for the [ruleSet] inside of an
@media directive; if found then replace
On 2013/09/18 22:40:54, nweiz wrote:
> "a @media"

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:794: return visitor._foundAndReplaced;
On 2013/09/18 22:40:54, nweiz wrote:
> This is also never used.

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:807: if (decl.include == _include) {
While re-writing not an issue anymore.
On 2013/09/18 22:40:54, nweiz wrote:
> Use "&&" rather than nested if statements.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:813: }
ditto on re-write.On 2013/09/18 22:40:54, nweiz wrote:
> This i++/for/break stuff is really complicated and completely obscures your
> intention. Add a utility function [indexWhere] that takes a list and a
function
> and returns the first index of an element in the iterable for which that
> function returns true.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:815: if (index != -1) {
ditto re-wrote not an issue.
On 2013/09/18 22:40:54, nweiz wrote:
> Use null rather than -1 as a flag value.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:834: _messages.warning("@mixin ${node.name} is
undefined.", node.span);
Class removed.
On 2013/09/18 22:40:54, nweiz wrote:
> This should be an error.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/analyze...
pkg/csslib/lib/src/analyzer.dart:844: class RemoveMixinsAndIncludes extends
Visitor {
class was eliminated when cycles removed.
On 2013/09/18 22:40:54, nweiz wrote:
> Don't the other visitors remove the mixins and includes? Why are you adding
> noops if they're just going to be removed?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
File pkg/csslib/lib/src/css_printer.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
pkg/csslib/lib/src/css_printer.dart:10: class CssPrinter extends Visitor {
Good point done for completness @mixin and @include should occur if analyzer has
been run.
On 2013/09/18 22:40:54, nweiz wrote:
> If this is printing the CSS tree, why does it need to print mixins? If it's
> printing a string representation of the SCSS tree, the name and documentation
> should indicate that.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
pkg/csslib/lib/src/css_printer.dart:209: if (topLevel) emit(_newLine);    //
Emit newline for top-level @include.
On 2013/09/18 22:40:54, nweiz wrote:
> Unnecessary spaces before the comment.

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
pkg/csslib/lib/src/css_printer.dart:211: if (topLevel) emit(';');
removed test.On 2013/09/18 22:40:54, nweiz wrote:
> Why wouldn't non-top-level @includes have a semicolon?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/css_pri...
pkg/csslib/lib/src/css_printer.dart:214: void
visitContentDirective(ContentDirective node) {
Place holder have errors for content directive in parser.  Don't want to forget
content printer.
On 2013/09/18 22:40:54, nweiz wrote:
> If you don't support content directives yet, just don't include the code.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/tree.dart
File pkg/csslib/lib/src/tree.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/tree.da...
pkg/csslib/lib/src/tree.dart:506: final bool varArgs;
args supported.
On 2013/09/18 22:40:54, nweiz wrote:
> As with @content, don't include infrastructure for features that aren't
> supported. You can add these once you actually support arguments.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/lib/src/tree.da...
pkg/csslib/lib/src/tree.dart:514: /** To support defining a SASS @mixin. */
Ok, referenced web site no need to explain the Sass world again.
On 2013/09/18 22:40:54, nweiz wrote:
> Only the first letter of "Sass" is capitalized.
> 
> These documentation comments are way too short. If I'm reading this as someone
> who doesn't know what a mixin is, I'm completely befuddled. Actually explain
> what a mixin is. Use complete sentences. If you have multiple classes for
> similar things -- IncludeDirective vs IncludeMixinAtDeclaration, for example
--
> explain the difference.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
File pkg/csslib/test/mixin_test.dart (right):

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:10: var  options = ['--warnings_as_errors',
'--no-colors', 'memory'];
On 2013/09/18 22:40:54, nweiz wrote:
> "final", not "var".

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:14: final input1 = r'''
Probably other test at one point. 1 is gone.
On 2013/09/18 22:40:54, nweiz wrote:
> Why is this [input1]? There's no other [input] variables in this scope. Same
> goes for [generated1] and [stylesheet1].
> 
> This should also be "var", not "final", since it's a local variable.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:23: ''';
I'll consider it but I fine pre-processing to be a pain in test too.
On 2013/09/18 22:40:54, nweiz wrote:
> These no-indentation strings are difficult to read, especially when
[generated]
> starts on the same line as the string opener. One thing Bob and I have done in
> the past is do a small amount of pre-processing to allow the input and output
to
> be indented eight spaces in the source of the test.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:30: final stylesheet1 = compileCss(input1,
errors: errors, opts: options);
It's something we've liked so far for tooling.
On 2013/09/18 22:40:54, nweiz wrote:
> Using an out parameter like [errors] here is a code smell. Why isn't
> [compileCss] just throwing an exception?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:36: // TODO(terry): Test using a declaration
@mixin as a top-level @include.
comment old.
On 2013/09/18 22:40:54, nweiz wrote:
> Why is this a TODO and not an actual test?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:74: void topLevelMixinMultiRulesets() {
Tests top-level mixins that includes another mixin
On 2013/09/18 22:40:54, nweiz wrote:
> What do multiple rulesets have to do with this test? It seems like the last
test
> tests multiple rulesets. This looks like it's testing transitive mixin
> inclusion. If that's the case, it should get rid of "@mixin a", as that's not
> contributing to the behavior being tested.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:120: void topLevelMixinMultiRulesets2() {
1, 2 and 3 nesting levels of includes.
On 2013/09/18 22:40:54, nweiz wrote:
> I have no idea what this test is testing. Is it just... a lot of mixins? Why
> would six mixins fail when two mixins didn't? This test seems superfluous.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:193: void topLevelMixinSelectors() {
testing with combinators and selector groups.
On 2013/09/18 22:40:54, nweiz wrote:
> All the previous tests had selectors. Saying that this one tests selectors
> doesn't tell me anything. Maybe this should be titled "test top-level mixin
with
> multiple selectors"?

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:275: void declMixinNestedIncludes() {
As test implies for than one level of testing.
On 2013/09/18 22:40:54, nweiz wrote:
> As above, "div-border" is unrelated to what this test is testing.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:312: void declMixinNestedIncludes2() {
nesting with other selectors pseudo.
On 2013/09/18 22:40:54, nweiz wrote:
> I can't understand what this test tests that the previous test didn't.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:354: @mixin div-inside(@dist) {
Vars are now different in CSS (var-) but we're support @ as well.
On 2013/09/18 22:40:54, nweiz wrote:
> If your arguments are going to use this syntax, you're not implementing SCSS,
> you're writing your own idiosyncratic stylesheet language. That's fine if you
> don't mind sacrificing user knowledge and cross-compatibility, but if you go
> that route, you can't refer to them as "Sass mixins" or use ".scss" as the
file
> extension.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:549: void badDeclarationInclude() {
Seems weird that top-level will close a declaration automatically.

span {
  border: 2px dashed red;
}
span #foo-id {
  color: red;
}

I'll consider matching this weird feature.

On 2013/09/18 22:40:54, nweiz wrote:
> This shouldn't cause an error.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:577: void badTopInclude() {
Currently, we really report everything as a warning and recover.
On 2013/09/18 22:40:54, nweiz wrote:
> This *should* cause an error.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:622: expect(errors.isNotEmpty, true, reason:
errors.toString());
test is gone.
On 2013/09/18 22:40:54, nweiz wrote:
> expect(errors, isNotEmpty)
> 
> [reason] here doesn't do anything, since it'll only be used when errors is
empty
> anyway.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:662: test('top-level mixin 2 @includes',
topLevelMixin2Includes);
On 2013/09/18 22:40:54, nweiz wrote:
> "2" -> "two", here and elsewhere. Also, at least use sentence fragments for
test
> descriptions (e.g. "top-level mixin with two @includes").

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:670: test('declaration mixin includes #2',
declMixinNestedIncludes2);
On 2013/09/18 22:40:54, nweiz wrote:
> Adding "#2" here doesn't tell me anything about the difference between the two
> tests.  Be more descriptive in your test descriptions.

Done.

https://codereview.chromium.org/23819036/diff/3001/pkg/csslib/test/mixin_test...
pkg/csslib/test/mixin_test.dart:677: // TODO(Terry): More tests to enable.
On 2013/09/18 22:40:54, nweiz wrote:
> Don't include tests that don't work. You can add these later along with the
> feature that makes them work. This is what source control is for.

Done.