From f82a1359ab518edeb4bb34c4e94a31747a5f7185 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Mon, 17 Jun 2019 16:46:02 -0700 Subject: [PATCH 1/5] Warn instead of crashing for bad URLs --- lib/src/migration_visitor.dart | 16 ++++++++++++---- lib/src/migrator.dart | 4 +++- lib/src/migrators/division.dart | 3 ++- lib/src/migrators/module.dart | 10 +++++++--- lib/src/runner.dart | 2 +- lib/src/utils.dart | 25 +++++++++++++++++++------ test/migrators/division/bad_paths.hrx | 13 +++++++++++++ 7 files changed, 57 insertions(+), 16 deletions(-) create mode 100644 test/migrators/division/bad_paths.hrx diff --git a/lib/src/migration_visitor.dart b/lib/src/migration_visitor.dart index 195d2a1d..0ed0a4cd 100644 --- a/lib/src/migration_visitor.dart +++ b/lib/src/migration_visitor.dart @@ -13,6 +13,7 @@ import 'package:sass/src/ast/sass.dart'; import 'package:sass/src/visitor/recursive_ast.dart'; import 'package:meta/meta.dart'; +import 'package:source_span/source_span.dart'; import 'patch.dart'; import 'utils.dart'; @@ -66,10 +67,16 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { } /// Visits the stylesheet at [dependency], resolved relative to [source]. + /// + /// This returns true if the dependency is successfully visited and false + /// otherwise. @protected - void visitDependency(Uri dependency, Uri source) { - var stylesheet = parseStylesheet(source.resolveUri(dependency)); + bool visitDependency(Uri dependency, Uri source, {FileSpan context}) { + var stylesheet = + parseStylesheet(source.resolveUri(dependency), context: context); + if (stylesheet == null) return false; visitStylesheet(stylesheet); + return true; } /// Returns the migrated contents of this file, or null if the file does not @@ -96,7 +103,8 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { if (migrateDependencies) { for (var import in node.imports) { if (import is DynamicImport) { - visitDependency(Uri.parse(import.url), node.span.sourceUrl); + visitDependency(Uri.parse(import.url), node.span.sourceUrl, + context: import.span); } } } @@ -108,7 +116,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { visitUseRule(UseRule node) { super.visitUseRule(node); if (migrateDependencies) { - visitDependency(node.url, node.span.sourceUrl); + visitDependency(node.url, node.span.sourceUrl, context: node.span); } } } diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index 34ee257c..90f1f22b 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -45,7 +45,9 @@ abstract class Migrator extends Command> { Map run() { var allMigrated = Map(); for (var entrypoint in argResults.rest) { - var migrated = migrateFile(canonicalize(Uri.parse(entrypoint))); + var canonicalUrl = canonicalize(Uri.parse(entrypoint)); + if (canonicalUrl == null) continue; + var migrated = migrateFile(canonicalUrl); for (var file in migrated.keys) { if (allMigrated.containsKey(file) && migrated[file] != allMigrated[file]) { diff --git a/lib/src/migrators/division.dart b/lib/src/migrators/division.dart index 0e7014de..97a7b903 100644 --- a/lib/src/migrators/division.dart +++ b/lib/src/migrators/division.dart @@ -199,7 +199,8 @@ class _DivisionMigrationVisitor extends MigrationVisitor { expectsNumericResult: true); return true; } else { - emitWarning("Could not determine whether this is division", node.span); + emitWarning("Could not determine whether this is division", + context: node.span); super.visitBinaryOperationExpression(node); return false; } diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index fa760e81..4cb8a13c 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -153,7 +153,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { if (nameArgument is! StringExpression || (nameArgument as StringExpression).text.asPlain == null) { emitWarning("get-function call may require \$module parameter", - nameArgument.span); + context: nameArgument.span); return; } var fnName = nameArgument as StringExpression; @@ -206,7 +206,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor { existingArgName: _findArgNameSpan(arg)); name = 'adjust'; } else { - emitWarning("Could not migrate malformed '$name' call", node.span); + emitWarning("Could not migrate malformed '$name' call", + context: node.span); return; } } @@ -270,7 +271,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor { var oldConfiguredVariables = _configuredVariables; _configuredVariables = Set(); - visitDependency(Uri.parse(import.url), _currentUrl); + if (!visitDependency(Uri.parse(import.url), _currentUrl, + context: import.span)) { + return; + } _namespaces[_lastUrl] = namespaceForPath(import.url); // Pass the variables that were configured by the importing file to `with`, diff --git a/lib/src/runner.dart b/lib/src/runner.dart index 549503d9..bd5c3e46 100644 --- a/lib/src/runner.dart +++ b/lib/src/runner.dart @@ -78,7 +78,7 @@ class MigratorRunner extends CommandRunner> { for (var url in migrated.keys) { assert(url.scheme == null || url.scheme == "file", "$url is not a file path."); - if (argResults['verbose']) print("Mirating ${p.prettyUri(url)}"); + if (argResults['verbose']) print("Migrating ${p.prettyUri(url)}"); File(url.toFilePath()).writeAsStringSync(migrated[url]); } } diff --git a/lib/src/utils.dart b/lib/src/utils.dart index ad6e2777..70c9646e 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -4,6 +4,7 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. +import 'package:path/path.dart' as p; import 'package:source_span/source_span.dart'; // The sass package's API is not necessarily stable. It is being imported with @@ -21,11 +22,19 @@ import 'patch.dart'; final _filesystemImporter = FilesystemImporter('.'); /// Returns the canonical version of [url]. -Uri canonicalize(Uri url) => _filesystemImporter.canonicalize(url); +Uri canonicalize(Uri url, {FileSpan context}) { + var canonicalUrl = url == null ? null : _filesystemImporter.canonicalize(url); + if (canonicalUrl == null) { + emitWarning("Could not find Sass file at '${p.prettyUri(url)}'.", + context: context); + } + return canonicalUrl; +} /// Parses the file at [url] into a stylesheet. -Stylesheet parseStylesheet(Uri url) { - var canonicalUrl = _filesystemImporter.canonicalize(url); +Stylesheet parseStylesheet(Uri url, {FileSpan context}) { + var canonicalUrl = canonicalize(url, context: context); + if (canonicalUrl == null) return null; var result = _filesystemImporter.load(canonicalUrl); return Stylesheet.parse(result.contents, result.syntax, url: canonicalUrl); } @@ -58,9 +67,13 @@ Patch patchDelete(FileSpan span, {int start = 0, int end}) { span.file.span(span.start.offset + start, span.start.offset + end), ""); } -/// Emits a warning with [message] and [context]; -void emitWarning(String message, FileSpan context) { - print(context.message("WARNING - $message")); +/// Emits a warning with [message] and optionally [context]; +void emitWarning(String message, {FileSpan context}) { + if (context == null) { + print("WARNING - $message"); + } else { + print(context.message("WARNING - $message")); + } } /// An exception thrown by a migrator. diff --git a/test/migrators/division/bad_paths.hrx b/test/migrators/division/bad_paths.hrx new file mode 100644 index 00000000..bc272731 --- /dev/null +++ b/test/migrators/division/bad_paths.hrx @@ -0,0 +1,13 @@ +<==> arguments +--migrate-deps + +<==> input/entrypoint.scss +@import "does_not_exist"; + +<==> log.txt +line 1, column 9 of entrypoint.scss: WARNING - Could not find Sass file at 'does_not_exist'. + , +1 | @import "does_not_exist"; + | ^^^^^^^^^^^^^^^^ + ' +Nothing to migrate! From 627d0be53173911bfa3a2c5bc3b5f21b015b645c Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Tue, 18 Jun 2019 14:28:57 -0700 Subject: [PATCH 2/5] Combine warnings for division; error for module --- lib/src/migration_visitor.dart | 28 +++++++++++++------ lib/src/migrator.dart | 13 +++++++++ lib/src/migrators/division.dart | 5 ++-- lib/src/migrators/module.dart | 15 +++++----- lib/src/runner.dart | 11 ++++++-- lib/src/utils.dart | 16 +++-------- test/migrators/division/bad_paths.hrx | 7 ++--- test/migrators/division/bad_paths_verbose.hrx | 12 ++++++++ test/migrators/module/bad_paths.hrx | 11 ++++++++ 9 files changed, 81 insertions(+), 37 deletions(-) create mode 100644 test/migrators/division/bad_paths_verbose.hrx create mode 100644 test/migrators/module/bad_paths.hrx diff --git a/lib/src/migration_visitor.dart b/lib/src/migration_visitor.dart index 0ed0a4cd..677ee11e 100644 --- a/lib/src/migration_visitor.dart +++ b/lib/src/migration_visitor.dart @@ -4,6 +4,7 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. +import 'package:path/path.dart' as p; import 'dart:collection'; // The sass package's API is not necessarily stable. It is being imported with @@ -35,6 +36,9 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { /// True if dependencies should be migrated as well. final bool migrateDependencies; + /// List of warnings for dependency URLs that could not be resolved. + final missingDependencies = []; + /// The patches to be applied to the stylesheet being migrated. UnmodifiableListView get patches => UnmodifiableListView(_patches); List _patches; @@ -43,8 +47,12 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { /// Runs a new migration on [url] (and its dependencies, if /// [migrateDependencies] is true) and returns a map of migrated contents. - Map run(Uri url) { + /// + /// Any warnings encountered for missing dependencies will be added to + /// [missingDependencies]. + Map run(Uri url, List missingDependencies) { visitStylesheet(parseStylesheet(url)); + missingDependencies.addAll(this.missingDependencies); return _migrated; } @@ -71,10 +79,14 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { /// This returns true if the dependency is successfully visited and false /// otherwise. @protected - bool visitDependency(Uri dependency, Uri source, {FileSpan context}) { - var stylesheet = - parseStylesheet(source.resolveUri(dependency), context: context); - if (stylesheet == null) return false; + bool visitDependency(Uri dependency, Uri source, [FileSpan context]) { + var url = source.resolveUri(dependency); + var stylesheet = parseStylesheet(url); + if (stylesheet == null) { + missingDependencies.add('${p.prettyUri(url)} ' + '@${p.prettyUri(context.sourceUrl)}:${context.start.line + 1}'); + return false; + } visitStylesheet(stylesheet); return true; } @@ -103,8 +115,8 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { if (migrateDependencies) { for (var import in node.imports) { if (import is DynamicImport) { - visitDependency(Uri.parse(import.url), node.span.sourceUrl, - context: import.span); + visitDependency( + Uri.parse(import.url), node.span.sourceUrl, import.span); } } } @@ -116,7 +128,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { visitUseRule(UseRule node) { super.visitUseRule(node); if (migrateDependencies) { - visitDependency(node.url, node.span.sourceUrl, context: node.span); + visitDependency(node.url, node.span.sourceUrl, node.span); } } } diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index 90f1f22b..f69eaeaf 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -26,6 +26,9 @@ abstract class Migrator extends Command> { /// If true, dependencies will be migrated in addition to the entrypoints. bool get migrateDependencies => globalResults['migrate-deps'] as bool; + /// List of warnings for dependency URLs that could not be resolved. + final missingDependencies = []; + /// Runs this migrator on [entrypoint] (and its dependencies, if the /// --migrate-deps flag is passed). /// @@ -57,6 +60,16 @@ abstract class Migrator extends Command> { allMigrated[file] = migrated[file]; } } + if (missingDependencies.isNotEmpty) { + var count = missingDependencies.length; + emitWarning( + "$count dependenc${count == 1 ? 'y' : 'ies'} could not be found."); + if (globalResults['verbose'] as bool) { + for (var warning in missingDependencies) { + print(' $warning'); + } + } + } return allMigrated; } } diff --git a/lib/src/migrators/division.dart b/lib/src/migrators/division.dart index 97a7b903..d8ba873b 100644 --- a/lib/src/migrators/division.dart +++ b/lib/src/migrators/division.dart @@ -38,7 +38,7 @@ More info: https://sass-lang.com/d/slash-div"""; @override Map migrateFile(Uri entrypoint) => _DivisionMigrationVisitor(this.isPessimistic, migrateDependencies) - .run(entrypoint); + .run(entrypoint, missingDependencies); } class _DivisionMigrationVisitor extends MigrationVisitor { @@ -199,8 +199,7 @@ class _DivisionMigrationVisitor extends MigrationVisitor { expectsNumericResult: true); return true; } else { - emitWarning("Could not determine whether this is division", - context: node.span); + emitWarning("Could not determine whether this is division", node.span); super.visitBinaryOperationExpression(node); return false; } diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index 4cb8a13c..e022bf6d 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -34,7 +34,8 @@ class ModuleMigrator extends Migrator { /// If [migrateDependencies] is false, the migrator will still be run on /// dependencies, but they will be excluded from the resulting map. Map migrateFile(Uri entrypoint) { - var migrated = _ModuleMigrationVisitor().run(entrypoint); + var migrated = + _ModuleMigrationVisitor().run(entrypoint, missingDependencies); if (!migrateDependencies) { migrated.removeWhere((url, contents) => url != entrypoint); } @@ -153,7 +154,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { if (nameArgument is! StringExpression || (nameArgument as StringExpression).text.asPlain == null) { emitWarning("get-function call may require \$module parameter", - context: nameArgument.span); + nameArgument.span); return; } var fnName = nameArgument as StringExpression; @@ -206,8 +207,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { existingArgName: _findArgNameSpan(arg)); name = 'adjust'; } else { - emitWarning("Could not migrate malformed '$name' call", - context: node.span); + emitWarning("Could not migrate malformed '$name' call", node.span); return; } } @@ -271,9 +271,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor { var oldConfiguredVariables = _configuredVariables; _configuredVariables = Set(); - if (!visitDependency(Uri.parse(import.url), _currentUrl, - context: import.span)) { - return; + if (!visitDependency(Uri.parse(import.url), _currentUrl, import.span)) { + throw MigrationException( + "Error: Could not find Sass file at '${import.url}'.", + span: import.span); } _namespaces[_lastUrl] = namespaceForPath(import.url); diff --git a/lib/src/runner.dart b/lib/src/runner.dart index bd5c3e46..05d52213 100644 --- a/lib/src/runner.dart +++ b/lib/src/runner.dart @@ -13,6 +13,7 @@ import 'package:term_glyph/term_glyph.dart' as glyph; import 'io.dart'; import 'migrators/division.dart'; import 'migrators/module.dart'; +import 'utils.dart'; /// A command runner that runs a migrator based on provided arguments. class MigratorRunner extends CommandRunner> { @@ -55,8 +56,14 @@ class MigratorRunner extends CommandRunner> { if (argResults.wasParsed('unicode')) { glyph.ascii = !(argResults['unicode'] as bool); } - - var migrated = await runCommand(argResults); + Map migrated; + try { + migrated = await runCommand(argResults); + } on MigrationException catch (e) { + print(e); + print('Migration failed!'); + return; + } if (migrated == null) return; if (migrated.isEmpty) { diff --git a/lib/src/utils.dart b/lib/src/utils.dart index 70c9646e..5b8f66eb 100644 --- a/lib/src/utils.dart +++ b/lib/src/utils.dart @@ -4,7 +4,6 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'package:path/path.dart' as p; import 'package:source_span/source_span.dart'; // The sass package's API is not necessarily stable. It is being imported with @@ -22,18 +21,11 @@ import 'patch.dart'; final _filesystemImporter = FilesystemImporter('.'); /// Returns the canonical version of [url]. -Uri canonicalize(Uri url, {FileSpan context}) { - var canonicalUrl = url == null ? null : _filesystemImporter.canonicalize(url); - if (canonicalUrl == null) { - emitWarning("Could not find Sass file at '${p.prettyUri(url)}'.", - context: context); - } - return canonicalUrl; -} +Uri canonicalize(Uri url) => _filesystemImporter.canonicalize(url); /// Parses the file at [url] into a stylesheet. -Stylesheet parseStylesheet(Uri url, {FileSpan context}) { - var canonicalUrl = canonicalize(url, context: context); +Stylesheet parseStylesheet(Uri url) { + var canonicalUrl = canonicalize(url); if (canonicalUrl == null) return null; var result = _filesystemImporter.load(canonicalUrl); return Stylesheet.parse(result.contents, result.syntax, url: canonicalUrl); @@ -68,7 +60,7 @@ Patch patchDelete(FileSpan span, {int start = 0, int end}) { } /// Emits a warning with [message] and optionally [context]; -void emitWarning(String message, {FileSpan context}) { +void emitWarning(String message, [FileSpan context]) { if (context == null) { print("WARNING - $message"); } else { diff --git a/test/migrators/division/bad_paths.hrx b/test/migrators/division/bad_paths.hrx index bc272731..1f068be0 100644 --- a/test/migrators/division/bad_paths.hrx +++ b/test/migrators/division/bad_paths.hrx @@ -3,11 +3,8 @@ <==> input/entrypoint.scss @import "does_not_exist"; +@import "does_not_exist2"; <==> log.txt -line 1, column 9 of entrypoint.scss: WARNING - Could not find Sass file at 'does_not_exist'. - , -1 | @import "does_not_exist"; - | ^^^^^^^^^^^^^^^^ - ' +WARNING - 2 dependencies could not be found. Nothing to migrate! diff --git a/test/migrators/division/bad_paths_verbose.hrx b/test/migrators/division/bad_paths_verbose.hrx new file mode 100644 index 00000000..e14dffdb --- /dev/null +++ b/test/migrators/division/bad_paths_verbose.hrx @@ -0,0 +1,12 @@ +<==> arguments +--migrate-deps --verbose + +<==> input/entrypoint.scss +@import "does_not_exist"; +@import "does_not_exist2"; + +<==> log.txt +WARNING - 2 dependencies could not be found. + does_not_exist @entrypoint.scss:1 + does_not_exist2 @entrypoint.scss:2 +Nothing to migrate! diff --git a/test/migrators/module/bad_paths.hrx b/test/migrators/module/bad_paths.hrx new file mode 100644 index 00000000..9165833c --- /dev/null +++ b/test/migrators/module/bad_paths.hrx @@ -0,0 +1,11 @@ +<==> input/entrypoint.scss +@import "does_not_exist"; +@import "does_not_exist2"; + +<==> log.txt +line 1, column 9 of entrypoint.scss: Error: Could not find Sass file at 'does_not_exist'. + , +1 | @import "does_not_exist"; + | ^^^^^^^^^^^^^^^^ + ' +Migration failed! From fdf7c60d40ab2ee6fbdcc1d45fa50f3a9f13f675 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Tue, 18 Jun 2019 14:34:23 -0700 Subject: [PATCH 3/5] Error on invalid entrypoint --- lib/src/migrator.dart | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index f69eaeaf..8412cd8f 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -49,7 +49,10 @@ abstract class Migrator extends Command> { var allMigrated = Map(); for (var entrypoint in argResults.rest) { var canonicalUrl = canonicalize(Uri.parse(entrypoint)); - if (canonicalUrl == null) continue; + if (canonicalUrl == null) { + throw MigrationException( + "Error: Could not find Sass file at '$entrypoint'."); + } var migrated = migrateFile(canonicalUrl); for (var file in migrated.keys) { if (allMigrated.containsKey(file) && From ae8a6780cbed69fe0d80902313e06243461f6c0f Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Wed, 19 Jun 2019 14:58:14 -0700 Subject: [PATCH 4/5] Iterate based on review --- lib/src/migration_visitor.dart | 28 ++++++--------- lib/src/migrator.dart | 36 ++++++++++++++----- lib/src/migrators/division.dart | 10 ++++-- lib/src/migrators/module.dart | 23 ++++++++---- test/migrators/division/bad_paths.hrx | 2 ++ test/migrators/division/bad_paths_verbose.hrx | 13 +++++-- 6 files changed, 73 insertions(+), 39 deletions(-) diff --git a/lib/src/migration_visitor.dart b/lib/src/migration_visitor.dart index 677ee11e..4e787d34 100644 --- a/lib/src/migration_visitor.dart +++ b/lib/src/migration_visitor.dart @@ -4,7 +4,6 @@ // license that can be found in the LICENSE file or at // https://opensource.org/licenses/MIT. -import 'package:path/path.dart' as p; import 'dart:collection'; // The sass package's API is not necessarily stable. It is being imported with @@ -36,8 +35,10 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { /// True if dependencies should be migrated as well. final bool migrateDependencies; - /// List of warnings for dependency URLs that could not be resolved. - final missingDependencies = []; + /// Map of missing dependency URLs to the spans that import/use them. + UnmodifiableMapView get missingDependencies => + UnmodifiableMapView(_missingDependencies); + final _missingDependencies = {}; /// The patches to be applied to the stylesheet being migrated. UnmodifiableListView get patches => UnmodifiableListView(_patches); @@ -47,12 +48,8 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { /// Runs a new migration on [url] (and its dependencies, if /// [migrateDependencies] is true) and returns a map of migrated contents. - /// - /// Any warnings encountered for missing dependencies will be added to - /// [missingDependencies]. - Map run(Uri url, List missingDependencies) { + Map run(Uri url) { visitStylesheet(parseStylesheet(url)); - missingDependencies.addAll(this.missingDependencies); return _migrated; } @@ -75,20 +72,15 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { } /// Visits the stylesheet at [dependency], resolved relative to [source]. - /// - /// This returns true if the dependency is successfully visited and false - /// otherwise. @protected - bool visitDependency(Uri dependency, Uri source, [FileSpan context]) { + void visitDependency(Uri dependency, Uri source, [FileSpan context]) { var url = source.resolveUri(dependency); var stylesheet = parseStylesheet(url); - if (stylesheet == null) { - missingDependencies.add('${p.prettyUri(url)} ' - '@${p.prettyUri(context.sourceUrl)}:${context.start.line + 1}'); - return false; + if (stylesheet != null) { + visitStylesheet(stylesheet); + } else { + _missingDependencies[url] = context; } - visitStylesheet(stylesheet); - return true; } /// Returns the migrated contents of this file, or null if the file does not diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index 8412cd8f..272041cd 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -6,6 +6,8 @@ import 'package:args/command_runner.dart'; import 'package:meta/meta.dart'; +import 'package:path/path.dart' as p; +import 'package:source_span/source_span.dart'; import 'utils.dart'; @@ -26,8 +28,8 @@ abstract class Migrator extends Command> { /// If true, dependencies will be migrated in addition to the entrypoints. bool get migrateDependencies => globalResults['migrate-deps'] as bool; - /// List of warnings for dependency URLs that could not be resolved. - final missingDependencies = []; + /// Map of missing dependency URLs to the spans that import/use them. + final missingDependencies = {}; /// Runs this migrator on [entrypoint] (and its dependencies, if the /// --migrate-deps flag is passed). @@ -53,6 +55,7 @@ abstract class Migrator extends Command> { throw MigrationException( "Error: Could not find Sass file at '$entrypoint'."); } + var migrated = migrateFile(canonicalUrl); for (var file in migrated.keys) { if (allMigrated.containsKey(file) && @@ -63,16 +66,33 @@ abstract class Migrator extends Command> { allMigrated[file] = migrated[file]; } } - if (missingDependencies.isNotEmpty) { + + if (missingDependencies.isNotEmpty) _warnForMissingDependencies(); + return allMigrated; + } + + /// Prints warnings for any missing dependencies encountered during migration. + /// + /// By default, this prints a short warning with one line per missing + /// dependency. + /// + /// In verbose mode, this instead prints a full warning with the source span + /// for each missing dependency. + void _warnForMissingDependencies() { + if (globalResults['verbose'] as bool) { + for (var uri in missingDependencies.keys) { + emitWarning("Could not find Sass file at '${p.prettyUri(uri)}'.", + missingDependencies[uri]); + } + } else { var count = missingDependencies.length; emitWarning( "$count dependenc${count == 1 ? 'y' : 'ies'} could not be found."); - if (globalResults['verbose'] as bool) { - for (var warning in missingDependencies) { - print(' $warning'); - } + for (var uri in missingDependencies.keys) { + var context = missingDependencies[uri]; + print(' ${p.prettyUri(uri)} ' + '@${p.prettyUri(context.sourceUrl)}:${context.start.line + 1}'); } } - return allMigrated; } } diff --git a/lib/src/migrators/division.dart b/lib/src/migrators/division.dart index d8ba873b..713d20d5 100644 --- a/lib/src/migrators/division.dart +++ b/lib/src/migrators/division.dart @@ -36,9 +36,13 @@ More info: https://sass-lang.com/d/slash-div"""; bool get isPessimistic => argResults['pessimistic'] as bool; @override - Map migrateFile(Uri entrypoint) => - _DivisionMigrationVisitor(this.isPessimistic, migrateDependencies) - .run(entrypoint, missingDependencies); + Map migrateFile(Uri entrypoint) { + var visitor = + _DivisionMigrationVisitor(this.isPessimistic, migrateDependencies); + var result = visitor.run(entrypoint); + missingDependencies.addAll(visitor.missingDependencies); + return result; + } } class _DivisionMigrationVisitor extends MigrationVisitor { diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index e022bf6d..436892fc 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -34,8 +34,7 @@ class ModuleMigrator extends Migrator { /// If [migrateDependencies] is false, the migrator will still be run on /// dependencies, but they will be excluded from the resulting map. Map migrateFile(Uri entrypoint) { - var migrated = - _ModuleMigrationVisitor().run(entrypoint, missingDependencies); + var migrated = _ModuleMigrationVisitor().run(entrypoint); if (!migrateDependencies) { migrated.removeWhere((url, contents) => url != entrypoint); } @@ -108,6 +107,20 @@ class _ModuleMigrationVisitor extends MigrationVisitor { return uses.join() + results; } + /// Visits the stylesheet at [dependency], resolved relative to [source]. + @override + void visitDependency(Uri dependency, Uri source, [FileSpan context]) { + var url = source.resolveUri(dependency); + var stylesheet = parseStylesheet(url); + if (stylesheet == null) { + throw MigrationException( + "Error: Could not find Sass file at '${p.prettyUri(url)}'.", + span: context); + } + + visitStylesheet(stylesheet); + } + /// Stores per-file state before visiting [node] and restores it afterwards. @override void visitStylesheet(Stylesheet node) { @@ -271,11 +284,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { var oldConfiguredVariables = _configuredVariables; _configuredVariables = Set(); - if (!visitDependency(Uri.parse(import.url), _currentUrl, import.span)) { - throw MigrationException( - "Error: Could not find Sass file at '${import.url}'.", - span: import.span); - } + visitDependency(Uri.parse(import.url), _currentUrl, import.span); _namespaces[_lastUrl] = namespaceForPath(import.url); // Pass the variables that were configured by the importing file to `with`, diff --git a/test/migrators/division/bad_paths.hrx b/test/migrators/division/bad_paths.hrx index 1f068be0..562bc89a 100644 --- a/test/migrators/division/bad_paths.hrx +++ b/test/migrators/division/bad_paths.hrx @@ -7,4 +7,6 @@ <==> log.txt WARNING - 2 dependencies could not be found. + does_not_exist @entrypoint.scss:1 + does_not_exist2 @entrypoint.scss:2 Nothing to migrate! diff --git a/test/migrators/division/bad_paths_verbose.hrx b/test/migrators/division/bad_paths_verbose.hrx index e14dffdb..067ca03c 100644 --- a/test/migrators/division/bad_paths_verbose.hrx +++ b/test/migrators/division/bad_paths_verbose.hrx @@ -6,7 +6,14 @@ @import "does_not_exist2"; <==> log.txt -WARNING - 2 dependencies could not be found. - does_not_exist @entrypoint.scss:1 - does_not_exist2 @entrypoint.scss:2 +line 1, column 9 of entrypoint.scss: WARNING - Could not find Sass file at 'does_not_exist'. + , +1 | @import "does_not_exist"; + | ^^^^^^^^^^^^^^^^ + ' +line 2, column 9 of entrypoint.scss: WARNING - Could not find Sass file at 'does_not_exist2'. + , +2 | @import "does_not_exist2"; + | ^^^^^^^^^^^^^^^^^ + ' Nothing to migrate! From 56eded3904749138cae06bc912c563c36a167566 Mon Sep 17 00:00:00 2001 From: Jennifer Thakar Date: Wed, 19 Jun 2019 16:25:34 -0700 Subject: [PATCH 5/5] Resolve remaining comments --- lib/src/migration_visitor.dart | 6 +++--- lib/src/migrator.dart | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/src/migration_visitor.dart b/lib/src/migration_visitor.dart index 4e787d34..da82987f 100644 --- a/lib/src/migration_visitor.dart +++ b/lib/src/migration_visitor.dart @@ -36,12 +36,12 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { final bool migrateDependencies; /// Map of missing dependency URLs to the spans that import/use them. - UnmodifiableMapView get missingDependencies => + Map get missingDependencies => UnmodifiableMapView(_missingDependencies); final _missingDependencies = {}; /// The patches to be applied to the stylesheet being migrated. - UnmodifiableListView get patches => UnmodifiableListView(_patches); + List get patches => UnmodifiableListView(_patches); List _patches; MigrationVisitor({this.migrateDependencies = true}); @@ -79,7 +79,7 @@ abstract class MigrationVisitor extends RecursiveAstVisitor { if (stylesheet != null) { visitStylesheet(stylesheet); } else { - _missingDependencies[url] = context; + _missingDependencies.putIfAbsent(url, () => context); } } diff --git a/lib/src/migrator.dart b/lib/src/migrator.dart index 272041cd..96c039b6 100644 --- a/lib/src/migrator.dart +++ b/lib/src/migrator.dart @@ -29,6 +29,11 @@ abstract class Migrator extends Command> { bool get migrateDependencies => globalResults['migrate-deps'] as bool; /// Map of missing dependency URLs to the spans that import/use them. + /// + /// Subclasses should add any missing dependencies to this when they are + /// encountered during migration. If using [MigrationVisitor], all items in + /// its `missingDependencies` property should be added to this after calling + /// `run`. final missingDependencies = {}; /// Runs this migrator on [entrypoint] (and its dependencies, if the