From dd3bc854d312a950ce38df6b1fc3d00bf5ba3075 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Wed, 15 May 2019 15:48:27 -0700 Subject: [PATCH] Improvements to the division migrator (#41) * Improvements to the division migrator Resolves #32 and resolves #33. * Refactor special color function handle into fn * Fix remaining comments * Add comment and changelog entry --- CHANGELOG.md | 7 ++ lib/src/migrators/division.dart | 95 +++++++++++++++++++-- pubspec.yaml | 4 +- test/migrators/division/always_division.hrx | 20 +++++ test/migrators/division/maybe_division.hrx | 2 + test/migrators/division/never_division.hrx | 23 ++--- test/migrators/division/slash_list.hrx | 17 ++++ 7 files changed, 143 insertions(+), 25 deletions(-) create mode 100644 test/migrators/division/slash_list.hrx diff --git a/CHANGELOG.md b/CHANGELOG.md index 76727a5a..b7a76b9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## 1.0.0-alpha.3 + +* Division migrator: Treat most slash operations within argument lists as + division. +* Division migrator: Only migrate slash operations to slash-list calls in a + non-plain-CSS context. + ## 1.0.0-alpha.2 * Internal changes only. diff --git a/lib/src/migrators/division.dart b/lib/src/migrators/division.dart index ce790bf7..f7e3d7fa 100644 --- a/lib/src/migrators/division.dart +++ b/lib/src/migrators/division.dart @@ -5,6 +5,7 @@ // https://opensource.org/licenses/MIT. import 'package:args/args.dart'; +import 'package:sass/sass.dart'; // The sass package's API is not necessarily stable. It is being imported with // the Sass team's explicit knowledge and approval. See @@ -49,6 +50,13 @@ class _DivisionMigrationVisitor extends MigrationVisitor { /// True when the current node is expected to evaluate to a number. var _expectsNumericResult = false; + /// Allows division within this argument invocation. + @override + void visitArgumentInvocation(ArgumentInvocation invocation) { + _withContext(() => super.visitArgumentInvocation(invocation), + isDivisionAllowed: true); + } + /// If this is a division operation, migrates it. /// /// If this is any other operator, allows division within its left and right @@ -65,6 +73,15 @@ class _DivisionMigrationVisitor extends MigrationVisitor { } } + /// Allows division within a function call's arguments, with special handling + /// for new-syntax color functions. + @override + void visitFunctionExpression(FunctionExpression node) { + visitInterpolation(node.name); + if (_tryColorFunction(node)) return; + visitArgumentInvocation(node.arguments); + } + /// Disallows division within this list. @override void visitListExpression(ListExpression node) { @@ -105,6 +122,48 @@ class _DivisionMigrationVisitor extends MigrationVisitor { isDivisionAllowed: true); } + /// Migrates [node] and returns true if it is a new-syntax color function or + /// returns false if it is any other function. + bool _tryColorFunction(FunctionExpression node) { + if (!["rgb", "rgba", "hsl", "hsla"].contains(node.name.asPlain)) { + return false; + } + + ListExpression channels; + if (node.arguments.positional.length == 1 && + node.arguments.named.isEmpty && + node.arguments.positional.first is ListExpression) { + channels = node.arguments.positional.first; + } else if (node.arguments.positional.isEmpty && + node.arguments.named.containsKey(r'$channels') && + node.arguments.named.length == 1 && + node.arguments.named[r'$channels'] is ListExpression) { + channels = node.arguments.named[r'$channels']; + } + if (channels == null || + channels.hasBrackets || + channels.separator != ListSeparator.space || + channels.contents.length != 3 || + channels.contents.last is! BinaryOperationExpression) { + return false; + } + + var last = channels.contents.last as BinaryOperationExpression; + if (last.left is! NumberExpression || last.right is! NumberExpression) { + // Handles cases like `rgb(10 20 30/2 / 0.5)`, since converting `30/2` to + // `divide(30, 20)` would cause `/ 0.5` to be interpreted as division. + _patchSpacesToCommas(channels); + _patchOperatorToComma(last); + } + _withContext(() { + channels.contents[0].accept(this); + channels.contents[1].accept(this); + last.left.accept(this); + }, isDivisionAllowed: true); + last.right.accept(this); + return true; + } + /// Visits a `/` operation [node] and migrates it to either the `division` /// function or the `slash-list` function. /// @@ -115,9 +174,14 @@ class _DivisionMigrationVisitor extends MigrationVisitor { if ((!_isDivisionAllowed && _onlySlash(node)) || _isDefinitelyNotNumber(node)) { // Definitely not division - addPatch(patchBefore(node, "slash-list(")); - addPatch(patchAfter(node, ")")); - _visitSlashListArguments(node); + if (_isDivisionAllowed || _containsInterpolation(node)) { + // We only want to convert a non-division slash operation to a + // slash-list call when it's in a non-plain-CSS context to avoid + // unnecessary function calls within plain CSS. + addPatch(patchBefore(node, "slash-list(")); + addPatch(patchAfter(node, ")")); + _visitSlashListArguments(node); + } return true; } else if (_expectsNumericResult || _isDefinitelyNumber(node) || @@ -126,7 +190,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor { addPatch(patchBefore(node, "divide(")); addPatch(patchAfter(node, ")")); _patchParensIfAny(node.left); - _patchSlashToComma(node); + _patchOperatorToComma(node); _patchParensIfAny(node.right); _withContext(() => super.visitBinaryOperationExpression(node), expectsNumericResult: true); @@ -145,7 +209,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor { if (node is BinaryOperationExpression && node.operator == BinaryOperator.dividedBy) { _visitSlashListArguments(node.left); - _patchSlashToComma(node); + _patchOperatorToComma(node); _visitSlashListArguments(node.right); } else if (node is StringExpression && node.text.contents.length == 1 && @@ -218,8 +282,27 @@ class _DivisionMigrationVisitor extends MigrationVisitor { node is StringExpression; } + /// Returns true if [node] contains an interpolation. + bool _containsInterpolation(Expression node) { + if (node is ParenthesizedExpression) return _containsInterpolation(node); + if (node is BinaryOperationExpression) { + return _containsInterpolation(node.left) || + _containsInterpolation(node.right); + } + return node is StringExpression && node.text.asPlain == null; + } + + /// Converts a space-separated list [node] to a comma-separated list. + void _patchSpacesToCommas(ListExpression node) { + for (var i = 0; i < node.contents.length - 1; i++) { + var start = node.contents[i].span.end; + var end = node.contents[i + 1].span.start; + addPatch(Patch(start.file.span(start.offset, end.offset), ", ")); + } + } + /// Adds a patch replacing the operator of [node] with ", ". - void _patchSlashToComma(BinaryOperationExpression node) { + void _patchOperatorToComma(BinaryOperationExpression node) { var start = node.left.span.end; var end = node.right.span.start; addPatch(Patch(start.file.span(start.offset, end.offset), ", ")); diff --git a/pubspec.yaml b/pubspec.yaml index 4f947cb2..03183b42 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,8 +1,8 @@ name: sass_migrator -version: 1.0.0-alpha.2 +version: 1.0.0-alpha.3 description: A tool for running migrations on Sass files author: Jennifer Thakar -homepage: https://github.com/sass/sass_migrator +homepage: https://github.com/sass/migrator environment: sdk: '>=2.2.0 <3.0.0' diff --git a/test/migrators/division/always_division.hrx b/test/migrators/division/always_division.hrx index 6e4eac0a..32312779 100644 --- a/test/migrators/division/always_division.hrx +++ b/test/migrators/division/always_division.hrx @@ -6,7 +6,12 @@ @return 6px / 3px; } +@function identity($x) { + @return $x; +} + a { + // Arithmetic on number literals b: (4px + 2px) / 3px; c: 6px/3px + 1; d: (6px / 3px); @@ -24,6 +29,11 @@ a { l: 3 / $x > 1; m: 3 / $x <= 1; n: 3 / $x >= 1; + + // Function calls + o: identity(6px / 3px); + p: rgba(10, 20, 30/2, 0.5); + q: rgb(10 20 30/2 / 0.5); } <==> output/entrypoint.scss @@ -31,7 +41,12 @@ a { @return divide(6px, 3px); } +@function identity($x) { + @return $x; +} + a { + // Arithmetic on number literals b: divide(4px + 2px, 3px); c: divide(6px, 3px) + 1; d: divide(6px, 3px); @@ -49,4 +64,9 @@ a { l: divide(3, $x) > 1; m: divide(3, $x) <= 1; n: divide(3, $x) >= 1; + + // Function calls + o: identity(divide(6px, 3px)); + p: rgba(10, 20, divide(30, 2), 0.5); + q: rgb(10, 20, divide(30, 2), 0.5); } diff --git a/test/migrators/division/maybe_division.hrx b/test/migrators/division/maybe_division.hrx index 27e06d50..3d37a897 100644 --- a/test/migrators/division/maybe_division.hrx +++ b/test/migrators/division/maybe_division.hrx @@ -5,6 +5,7 @@ a { d: 3 / $x == 4; e: fn() / 3; f: 3 / $x; + g: fn(3 / $x); } <==> output/entrypoint.scss @@ -14,4 +15,5 @@ a { d: divide(3, $x) == 4; e: divide(fn(), 3); f: divide(3, $x); + g: fn(divide(3, $x)); } diff --git a/test/migrators/division/never_division.hrx b/test/migrators/division/never_division.hrx index 3da95023..c81d1ba7 100644 --- a/test/migrators/division/never_division.hrx +++ b/test/migrators/division/never_division.hrx @@ -2,22 +2,11 @@ :foo(#{6/3}) { b: 6px / 3px; c: 6px / 3px / 2; - d: #{4px + 2px} / 3px; - e: bold 6px/3px sans-serif; - f: (bold 6px/3px sans-serif); - g: (six / three); - h: 6 / three; - i: #{$x} / #{6px / 3px} / #{2}; + d: bold 6px/3px sans-serif; + e: (bold 6px/3px sans-serif); + f: 6 / three; + g: rgb(10 20 30 / 0.5); } -<==> output/entrypoint.scss -:foo(#{slash-list(6, 3)}) { - b: slash-list(6px, 3px); - c: slash-list(6px, 3px, 2); - d: slash-list(4px + 2px, 3px); - e: bold slash-list(6px, 3px) sans-serif; - f: (bold slash-list(6px, 3px) sans-serif); - g: slash-list(six, three); - h: slash-list(6, three); - i: slash-list($x, slash-list(6px, 3px), 2); -} +<==> log.txt +Nothing to migrate! diff --git a/test/migrators/division/slash_list.hrx b/test/migrators/division/slash_list.hrx new file mode 100644 index 00000000..468a6da5 --- /dev/null +++ b/test/migrators/division/slash_list.hrx @@ -0,0 +1,17 @@ +<==> input/entrypoint.scss +a { + b: #{4px + 2px} / 3px; + c: (six / three); + $d: 6 / three; + e: #{$x} / #{6px / 3px} / #{2}; + $f: #{$x} / #{6px / 3px} / #{2}; +} + +<==> output/entrypoint.scss +a { + b: slash-list(4px + 2px, 3px); + c: slash-list(six, three); + $d: slash-list(6, three); + e: slash-list($x, 6px / 3px, 2); + $f: slash-list($x, divide(6px, 3px), 2); +}