Skip to content
This repository has been archived by the owner on May 21, 2019. It is now read-only.

New rule: no_condition_parens_rule #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions lib/src/engine.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import 'linter.dart';
import 'rule.dart';
import 'rules/no_debug.dart';
import 'rules/no_empty_style.dart';
import 'rules/no_condition_parens.dart';
import 'rules/no_loud_comment.dart';
import 'rules/non_numeric_dimension.dart';
import 'rules/quote_map_keys.dart';
Expand All @@ -21,6 +22,7 @@ import 'rules/use_falsey_null.dart';
final allRules = <Rule>[
new NoDebugRule(),
new NoEmptyStyleRule(),
new NoConditionParensRule(),
new NoLoudCommentRule(),
new NonNumericDimensionRule(),
new QuoteMapKeysRule(),
Expand Down Expand Up @@ -86,7 +88,7 @@ class Engine {
}

Iterable<String> _scssFilesInDir(String path) => new Directory(path)
.listSync(recursive: true)
.where((entity) => entity is File)
.map((entity) => entity.path)
.where((path) => path.endsWith('.scss'));
.listSync(recursive: true)
.where((entity) => entity is File)
.map((entity) => entity.path)
.where((path) => path.endsWith('.scss'));
7 changes: 6 additions & 1 deletion lib/src/rule.dart
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,12 @@ abstract class Rule

@override
List<Lint> visitWhileRule(WhileRule node) {
throw new UnimplementedError();
var lint = <Lint>[];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could shorten this to look like, e.g. visitStylesheet.

lint.addAll(node.condition.accept(this));
for (var child in node.children) {
lint.addAll(child.accept(this));
}
return lint;
}

List<Lint> _visitInterpolation(Interpolation node) {
Expand Down
49 changes: 49 additions & 0 deletions lib/src/rules/no_condition_parens.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright 2018 Google Inc. Use of this source code is governed by an
// MIT-style license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

// The sass package's API is not necessarily stable. It is being imported with
// the Sass team's explicit knowledge and approval. See
// https://github.com/sass/dart-sass/issues/236.
import 'package:sass/src/ast/sass.dart';

import '../lint.dart';
import '../rule.dart';

/// A lint rule that reports unnecessary parentheses in conditionals.
///
/// Sometimes users who are familiar with other languages forget that Sass
/// doesn't need parentheses around its conditionals (as in, `@if (...) {`, or
/// `@else if (...) {`, or `@while (...) {`). This rule suggests removing these
/// parentheses.
class NoConditionParensRule extends Rule {
NoConditionParensRule() : super('no_condition_parens_rule');

@override
List<Lint> visitIfRule(IfRule node) {
var lint = <Lint>[];
for (var clause in node.clauses) {
if (clause.expression is ParenthesizedExpression) {
lint.add(Lint(
rule: this,
span: clause.expression.span,
message:
'Parentheses around ${clause.expression.span.text} are unnecessary'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid some wrapping here and lots of law-of-demeter, I'd recommend instantiating a variable either for clause.expression (above the if) or clause.expression.span (above the lint.add).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wrap ${clause.expression.span.text} in either single or double quotes. I forget what the convention is in other lints...

}
}
return lint..addAll(super.visitIfRule(node));
}

@override
List<Lint> visitWhileRule(WhileRule node) {
var lint = <Lint>[];
if (node.condition is ParenthesizedExpression) {
lint.add(Lint(
rule: this,
span: node.condition.span,
message:
'Parentheses around ${node.condition.span.text} are unnecessary'));
}
return lint..addAll(super.visitWhileRule(node));
}
}
26 changes: 25 additions & 1 deletion test/rule_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ void main() {
expect(lint.column, 27);
});

test('reports lint when boolean is found in a @for condition', () {
test('reports lint when numbers are found in a @for condition', () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha yes, thanks for fixing this. You will get some merge conflicts with #29 where I overhaul this file.

var lints = getLints(r'''
@for $i from 1 through 3 {
$a: red;
Expand Down Expand Up @@ -204,6 +204,30 @@ void main() {
expect(lint.column, 14);
});

test('reports lint when boolean is found in a @while condition', () {
var lints = getLints(r'''
@while false {
$a: red;
}
''');

expect(lints, hasLength(1));
expect(lints[0].line, 0);
expect(lints[0].column, 15);
});

test('reports lint when boolean is found in a @while body', () {
var lints = getLints(r'''
@while 'hello' != 'world' {
$a: true;
}
''');

expect(lints, hasLength(1));
expect(lints[0].line, 1);
expect(lints[0].column, 14);
});

test('reports lint when boolean is found in a @function body', () {
var lints = getLints(r'''
@function grid-width($n) {
Expand Down
67 changes: 67 additions & 0 deletions test/rules/no_condition_parens_test.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright 2018 Google Inc. Use of this source code is governed by an
// MIT-style license that can be found in the LICENSE file or at
// https://opensource.org/licenses/MIT.

import 'package:sass_linter/src/rules/no_condition_parens.dart';
import 'package:sass_linter/src/lint.dart';
import 'package:sass_linter/src/linter.dart';
import 'package:test/test.dart';

final url = 'a.scss';
final rule = new NoConditionParensRule();

void main() {
test('does not report lint when there are no parens', () {
var lints = getLints(r'@if 1 != 7 { @debug("quack"); }');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great tests! I'd love one more test with an inner expression that contains parens, like:

@if (a + b) * c < 3 { @debug("quack"); }


expect(lints, isEmpty);
});

test('reports lint when there is a paren in @if', () {
var lints = getLints(r'@if (1 != 7) { @debug("quack"); }');

expect(lints, hasLength(1));

var lint = lints.single;
expect(lint.rule, rule);
expect(lint.message, contains('Parentheses'));
expect(lint.message, contains('(1 != 7)'));
expect(lint.message, contains('unnecessary'));
expect(lint.url, new Uri.file(url));
expect(lint.line, 0);
expect(lint.column, 4);
});

test('reports lint when there is a paren in @else if', () {
var lints = getLints(r'@if 1 {} @else if (2 != 3) { @debug("quack"); }');

expect(lints, hasLength(1));

var lint = lints.single;
expect(lint.rule, rule);
expect(lint.message, contains('Parentheses'));
expect(lint.message, contains('(2 != 3)'));
expect(lint.message, contains('unnecessary'));
expect(lint.url, new Uri.file(url));
expect(lint.line, 0);
expect(lint.column, 18);
});

test('reports lint when there is a paren in @while', () {
var lints = getLints(r'@while (2 == 3) { @debug("quack"); }');

expect(lints, hasLength(1));

var lint = lints.single;
expect(lint.rule, rule);
expect(lint.message, contains('Parentheses'));
expect(lint.message, contains('(2 == 3)'));
expect(lint.message, contains('unnecessary'));
expect(lint.url, new Uri.file(url));
expect(lint.line, 0);
expect(lint.column, 18);
});
}

List<Lint> getLints(String source) =>
new Linter(source, [rule], url: url).run();