Skip to content

Commit

Permalink
Code review
Browse files Browse the repository at this point in the history
  • Loading branch information
@jathak
jathak committed Jan 4, 2024
1 parent a782328 commit 63280b3
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 95 deletions.
118 changes: 56 additions & 62 deletions lib/src/migrators/division.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,10 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
}

var channels = switch (node.arguments) {
ArgumentInvocation(positional: [ListExpression arg, ...], named: Map()) =>
ArgumentInvocation(
positional: [ListExpression arg],
named: Map(isEmpty: true)
) =>
arg,
ArgumentInvocation(
positional: [],
Expand All @@ -291,15 +294,15 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
)) {
// Handles cases like `rgb(10 20 30/2 / 0.5)`, since converting `30/2`
// to `div(30, 20)` would cause `/ 0.5` to be interpreted as division.
switch (last) {
case BinaryOperationExpression(
left: NumberExpression _,
right: NumberExpression _
):
break;
default:
_patchSpacesToCommas(channels);
_patchOperatorToComma(last);
if (last
case BinaryOperationExpression(
left: NumberExpression(),
right: NumberExpression()
)) {
// Nothing to patch
} else {
_patchSpacesToCommas(channels);
_patchOperatorToComma(last);
}
_withContext(() {
// Non-null assertion is required because of dart-lang/language#1536.
Expand Down Expand Up @@ -366,8 +369,8 @@ class _DivisionMigrationVisitor extends MigrationVisitor {
/// Returns true if patched and false otherwise.
bool _tryMultiplication(BinaryOperationExpression node) {
if (!useMultiplication) return false;
if (node.right case NumberExpression(unit: null, value: var divisor)) {
if (!_allowedDivisors.contains(divisor)) return false;
if (node.right case NumberExpression(unit: null, value: var divisor)
when _allowedDivisors.contains(divisor)) {
var operatorSpan = node.left.span
.extendThroughWhitespace()
.end
Expand Down Expand Up @@ -413,35 +416,27 @@ class _DivisionMigrationVisitor extends MigrationVisitor {

/// Returns true if [node] is entirely composed of number literals and slash
/// operations.
bool _onlySlash(Expression node) {
switch (node) {
case NumberExpression():
return true;
case BinaryOperationExpression(
bool _onlySlash(Expression node) => switch (node) {
NumberExpression() => true,
BinaryOperationExpression(
operator: BinaryOperator.dividedBy,
:var left,
:var right
):
return _onlySlash(left) && _onlySlash(right);
default:
return false;
}
}
) =>
_onlySlash(left) && _onlySlash(right),
_ => false
};

/// Returns true if [node] contains an interpolation.
bool _containsInterpolation(Expression node) {
switch (node) {
case ParenthesizedExpression(:var expression):
case UnaryOperationExpression(operand: var expression):
return _containsInterpolation(expression);
case BinaryOperationExpression(:var left, :var right):
return _containsInterpolation(left) || _containsInterpolation(right);
case StringExpression(text: Interpolation(asPlain: null)):
return true;
default:
return false;
}
}
bool _containsInterpolation(Expression node) => switch (node) {
ParenthesizedExpression(:var expression) ||
UnaryOperationExpression(operand: var expression) =>
_containsInterpolation(expression),
BinaryOperationExpression(:var left, :var right) =>
_containsInterpolation(left) || _containsInterpolation(right),
StringExpression(text: Interpolation(asPlain: null)) => true,
_ => false
};

/// Converts a space-separated list [node] to a comma-separated list.
void _patchSpacesToCommas(ListExpression node) {
Expand Down Expand Up @@ -498,31 +493,30 @@ enum _NumberStatus {

/// Returns [yes] if [node] is definitely a number, [no] if [node] is
/// definitely not a number, and [maybe] otherwise.
static _NumberStatus of(Expression node) {
switch (node) {
case NumberExpression():
case BinaryOperationExpression(
static _NumberStatus of(Expression node) => switch (node) {
NumberExpression() ||
BinaryOperationExpression(
operator: BinaryOperator.times || BinaryOperator.modulo
):
return yes;
case BooleanExpression():
case ColorExpression():
case ListExpression():
case MapExpression():
case NullExpression():
case StringExpression():
return no;
case ParenthesizedExpression(:var expression):
case UnaryOperationExpression(operand: var expression):
return of(expression);
case BinaryOperationExpression(:var left, :var right):
return switch ((of(left), of(right))) {
(yes, yes) => yes,
(no, _) || (_, no) => no,
_ => maybe
};
default:
return maybe;
}
}
) =>
yes,
BooleanExpression() ||
ColorExpression() ||
ListExpression() ||
MapExpression() ||
NullExpression() ||
StringExpression() =>
no,
ParenthesizedExpression(:var expression) ||
UnaryOperationExpression(operand: var expression) =>
of(expression),
BinaryOperationExpression(:var left, :var right) => switch ((
of(left),
of(right)
)) {
(yes, yes) => yes,
(no, _) || (_, no) => no,
_ => maybe
},
_ => maybe,
};
}
49 changes: 20 additions & 29 deletions lib/src/migrators/module.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,18 +315,13 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
var entrypointForwards = forwards != null
? _forwardRulesForShown(entrypoint, '"$dependency"', forwards, {})
: ['@forward "$dependency"'];
var tuples = [
for (var entry in forwardsByUrl.entries)
(
entry.key,
_absoluteUrlToDependency(entry.key, relativeTo: importOnlyUrl).$1,
entry.value
)
];
var forwardLines = [
for (var (url, ruleUrl, shownByPrefix) in tuples)
..._forwardRulesForShown(
url, '"${ruleUrl}"', shownByPrefix, hiddenByUrl[url] ?? {}),
for (var MapEntry(key: url, value: shownByPrefix)
in forwardsByUrl.entries)
...switch (_absoluteUrlToDependency(url, relativeTo: importOnlyUrl)) {
(var ruleUrl, _) => _forwardRulesForShown(
url, '"$ruleUrl"', shownByPrefix, hiddenByUrl[url] ?? {})
},
...entrypointForwards
];
var semicolon = entrypoint.path.endsWith('.sass') ? '' : ';';
Expand Down Expand Up @@ -377,10 +372,9 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
.map((declaration) => declaration.sourceUrl)
.toSet()) {
if (url == currentUrl || _forwardedUrls.contains(url)) continue;
var forwards =
_makeForwardRules(url, '"${_absoluteUrlToDependency(url).$1}"');
var (ruleUrl, isRelative) = _absoluteUrlToDependency(url);
var forwards = _makeForwardRules(url, '"$ruleUrl"');
if (forwards == null) continue;
var (_, isRelative) = _absoluteUrlToDependency(url);
(isRelative ? relativeForwards : loadPathForwards)
.addAll([for (var rule in forwards) '$rule$semicolon\n']);
}
Expand Down Expand Up @@ -766,22 +760,20 @@ class _ModuleMigrationVisitor extends MigrationVisitor {

for (var import in dynamicImports) {
Uri? ruleUrl = import.url;
var tuple = importCache.canonicalize(ruleUrl,
baseImporter: importer, baseUrl: currentUrl, forImport: true);
var canonicalImport = tuple?.$2;
if (canonicalImport != null &&
references.orphanImportOnlyFiles.containsKey(canonicalImport)) {
if (importCache.canonicalize(ruleUrl,
baseImporter: importer, baseUrl: currentUrl, forImport: true)
case (var newImporter, var canonicalImport, originalUrl: _)?
when references.orphanImportOnlyFiles.containsKey(canonicalImport)) {
ruleUrl = null;
var url = references.orphanImportOnlyFiles[canonicalImport]?.url;
if (url != null && tuple != null) {
var canonicalRedirect = importCache
.canonicalize(url,
baseImporter: tuple.$1, baseUrl: canonicalImport)!
.$2;
(ruleUrl, _) = _absoluteUrlToDependency(canonicalRedirect);
if (references.orphanImportOnlyFiles[canonicalImport]
case ForwardRule(:var url)) {
if (importCache.canonicalize(url,
baseImporter: newImporter, baseUrl: canonicalImport)
case (_, var canonicalRedirect, originalUrl: _)?) {
(ruleUrl, _) = _absoluteUrlToDependency(canonicalRedirect);
}
}
}

if (ruleUrl != null) {
if (_useAllowed) {
migratedRules.addAll(_migrateImportToRules(ruleUrl, import.span));
Expand Down Expand Up @@ -1266,8 +1258,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// false when it's resolved relative to a load path.
(Uri, bool) _absoluteUrlToDependency(Uri url, {Uri? relativeTo}) {
relativeTo ??= currentUrl;
var tuple = _originalImports[url];
if (tuple case (var url, NodeModulesImporter _)) {
if (_originalImports[url] case (var url, NodeModulesImporter _)) {
return (url, false);
}

Expand Down
9 changes: 5 additions & 4 deletions lib/src/util/scoped_ast_visitor.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import 'scope.dart';
/// current scope.
abstract class ScopedAstVisitor
with RecursiveStatementVisitor, RecursiveAstVisitor {
/// The current scope, containing any visible without a namespace to the
/// current point in the AST.
/// The current scope, containing any visible Sass members without a namespace
/// to the current point in the AST.
///
/// Subclasses that visit multiple modules should update this when changing
/// the module being visited.
Expand Down Expand Up @@ -64,8 +64,9 @@ abstract class ScopedAstVisitor
scoped(() {
for (var argument in node.arguments.arguments) {
currentScope.variables[argument.name] = MemberDeclaration(argument);
var defaultValue = argument.defaultValue;
if (defaultValue != null) visitExpression(defaultValue);
if (argument.defaultValue case var defaultValue?) {
visitExpression(defaultValue);
}
}
visitChildren(node.children, withScope: false);
});
Expand Down

0 comments on commit 63280b3

Please sign in to comment.