Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn instead of crashing for bad URLs #63

Merged
merged 5 commits into from
Jun 19, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
16 changes: 12 additions & 4 deletions lib/src/migration_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
}
}
Expand All @@ -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);
}
}
}
4 changes: 3 additions & 1 deletion lib/src/migrator.dart
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ abstract class Migrator extends Command<Map<Uri, String>> {
Map<Uri, String> run() {
var allMigrated = Map<Uri, String>();
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]) {
Expand Down
3 changes: 2 additions & 1 deletion lib/src/migrators/division.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
10 changes: 7 additions & 3 deletions lib/src/migrators/module.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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`,
Expand Down
2 changes: 1 addition & 1 deletion lib/src/runner.dart
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class MigratorRunner extends CommandRunner<Map<Uri, String>> {
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]);
}
}
Expand Down
25 changes: 19 additions & 6 deletions lib/src/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
Expand Down Expand Up @@ -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}) {
jathak marked this conversation as resolved.
Show resolved Hide resolved
if (context == null) {
print("WARNING - $message");
} else {
print(context.message("WARNING - $message"));
}
}

/// An exception thrown by a migrator.
Expand Down
13 changes: 13 additions & 0 deletions test/migrators/division/bad_paths.hrx
Original file line number Diff line number Diff line change
@@ -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!