Pubspec parsing error handling; name cleanup.

dart-lang · Mar 20, 2015 · a44ee83 · a44ee83
1 parent 5597e01
commit a44ee83
Show file tree

Hide file tree

Showing 9 changed files with 58 additions and 33 deletions.
diff --git a/lib/src/io.dart b/lib/src/io.dart
@@ -97,7 +97,7 @@ String _calculateProjectPackageName(String path) {
   var pubspec = _findPubspecFileRelativeTo(path);
   if (pubspec != null) {
     var spec =
-        new PubSpec.parse(pubspec.readAsStringSync(), sourceUrl: pubspec.path);
+        new Pubspec.parse(pubspec.readAsStringSync(), sourceUrl: pubspec.path);
     var nameEntry = spec.name;
     if (nameEntry != null) {
       var value = nameEntry.value.text;

diff --git a/lib/src/linter.dart b/lib/src/linter.dart
@@ -21,7 +21,7 @@ import 'package:linter/src/rules.dart';
 void _registerLinters(Iterable<Linter> linters) {
   if (linters != null) {
     LintGenerator.LINTERS.clear();
-    linters.forEach((l) => LintGenerator.LINTERS.add(l));
+    LintGenerator.LINTERS.addAll(linters);
   }
 }
 
@@ -260,7 +260,7 @@ abstract class LintRule extends Linter implements Comparable<LintRule> {
   /// Return a visitor to be passed to pubspecs to perform lint
   /// analysis.
   /// Lint errors are reported via this [Linter]'s error [reporter].
-  PubSpecVisitor getPubspecVisitor() => null;
+  PubspecVisitor getPubspecVisitor() => null;
 
   @override
   AstVisitor getVisitor() => null;
@@ -353,7 +353,7 @@ abstract class Reporter {
   void warn(String message);
 }
 
-/// Linter implementation
+/// Linter implementation.
 class SourceLinter implements DartLinter, AnalysisErrorListener {
   final errors = <AnalysisError>[];
   final LinterOptions options;
@@ -362,8 +362,7 @@ class SourceLinter implements DartLinter, AnalysisErrorListener {
   @override
   int numSourcesAnalyzed;
 
-  SourceLinter(LinterOptions options, {this.reporter: const PrintingReporter()})
-      : this.options = options != null ? options : _defaultOptions();
+  SourceLinter(this.options, {this.reporter: const PrintingReporter()});
 
   @override
   Iterable<AnalysisErrorInfo> lintFiles(List<File> files) {
@@ -385,7 +384,7 @@ class SourceLinter implements DartLinter, AnalysisErrorListener {
     var results = <AnalysisErrorInfo>[];
 
     //TODO: error handling
-    var spec = new PubSpec.parse(contents, sourceUrl: sourceUrl);
+    var spec = new Pubspec.parse(contents, sourceUrl: sourceUrl);
 
     for (Linter lint in options.enabledLints) {
       if (lint is LintRule) {
@@ -421,8 +420,6 @@ class SourceLinter implements DartLinter, AnalysisErrorListener {
   Iterable<AnalysisErrorInfo> _lintPubspecFile(File sourceFile) =>
       lintPubspecSource(
           contents: sourceFile.readAsStringSync(), sourceUrl: sourceFile.path);
-
-  static LinterOptions _defaultOptions() => new LinterOptions(ruleRegistry);
 }
 
 class _LineInfo implements LineInfo {

diff --git a/lib/src/pub.dart b/lib/src/pub.dart
@@ -118,9 +118,9 @@ abstract class PSNodeList extends Object with IterableMixin<PSNode> {
   PSNode get token;
 }
 
-abstract class PubSpec {
-  factory PubSpec.parse(String source, {String sourceUrl}) =>
-      new _PubSpec(source, sourceUrl: sourceUrl);
+abstract class Pubspec {
+  factory Pubspec.parse(String source, {String sourceUrl}) =>
+      new _Pubspec(source, sourceUrl: sourceUrl);
   PSEntry get author;
   PSNodeList get authors;
   PSDependencyList get dependencies;
@@ -130,10 +130,10 @@ abstract class PubSpec {
   PSEntry get homepage;
   PSEntry get name;
   PSEntry get version;
-  accept(PubSpecVisitor visitor);
+  accept(PubspecVisitor visitor);
 }
 
-abstract class PubSpecVisitor<T> {
+abstract class PubspecVisitor<T> {
   T visitPackageAuthor(PSEntry author) => null;
   T visitPackageAuthors(PSNodeList authors) => null;
   T visitPackageDependencies(PSDependencyList dependencies) => null;
@@ -175,7 +175,6 @@ class _PSDependency extends PSDependency {
       details.nodes.forEach((k, v) {
         if (k is! YamlScalar) {
           return;
-          //WARN?
         }
         YamlScalar key = k;
         switch (key.toString()) {
@@ -222,9 +221,9 @@ class _PSDependency extends PSDependency {
 }
 
 class _PSDependencyList extends PSDependencyList {
-  final PSNode token;
 
   final dependencies = <PSDependency>[];
+  final PSNode token;
 
   _PSDependencyList(this.token);
 
@@ -290,7 +289,7 @@ $token:
   - ${nodes.join('\n  - ')}''';
 }
 
-class _PubSpec implements PubSpec {
+class _Pubspec implements Pubspec {
   PSEntry author;
   PSNodeList authors;
   PSEntry description;
@@ -301,11 +300,15 @@ class _PubSpec implements PubSpec {
   PSDependencyList dependencies;
   PSDependencyList devDependencies;
 
-  _PubSpec(String src, {String sourceUrl}) {
-    _parse(src, sourceUrl: sourceUrl);
+  _Pubspec(String src, {String sourceUrl}) {
+    try {
+      _parse(src, sourceUrl: sourceUrl);
+    } on Exception {
+      // ignore
+    }
   }
 
-  void accept(PubSpecVisitor visitor) {
+  void accept(PubspecVisitor visitor) {
     if (author != null) {
       visitor.visitPackageAuthor(author);
     }
@@ -355,13 +358,11 @@ class _PubSpec implements PubSpec {
     var yaml = loadYamlNode(src, sourceUrl: sourceUrl);
     if (yaml is! YamlMap) {
       return;
-      // WARN?
     }
     YamlMap yamlMap = yaml;
     yamlMap.nodes.forEach((k, v) {
       if (k is! YamlScalar) {
         return;
-        //WARN?
       }
       YamlScalar key = k;
       switch (key.toString()) {

diff --git a/lib/src/rules/pub/package_names.dart b/lib/src/rules/pub/package_names.dart
@@ -35,10 +35,10 @@ class PubPackageNames extends LintRule {
           kind: Kind.DO);
 
   @override
-  PubSpecVisitor getPubspecVisitor() => new Visitor(this);
+  PubspecVisitor getPubspecVisitor() => new Visitor(this);
 }
 
-class Visitor extends PubSpecVisitor {
+class Visitor extends PubspecVisitor {
   LintRule rule;
   Visitor(this.rule);
 

diff --git a/test/_data/p3/_pubspec.yaml b/test/_data/p3/_pubspec.yaml
@@ -0,0 +1,4 @@
+not: a
+  valid
+ pub
+  spec:
diff --git a/test/integration_test.dart b/test/integration_test.dart
@@ -44,6 +44,21 @@ defineTests() {
         });
       });
     });
+    group('p3', () {
+      IOSink currentOut = outSink;
+      CollectingSink collectingOut = new CollectingSink();
+      setUp(() => outSink = collectingOut);
+      tearDown(() {
+        collectingOut.buffer.clear();
+        outSink = currentOut;
+      });
+      test('bad pubspec', () {
+        dartlint.main(['test/_data/p3', 'test/_data/p3/_pubpspec.yaml']);
+        expect(collectingOut.trim(),
+            endsWith('1 file analyzed, 0 issues found.'));
+      });
+    });
+
     group('examples', () {
       test('lintconfig.yaml', () {
         var src = readFile('example/lintconfig.yaml');

diff --git a/test/linter_test.dart b/test/linter_test.dart
@@ -604,7 +604,7 @@ class MockLinter extends LintRule {
   }
 
   @override
-  PubSpecVisitor getPubspecVisitor() => visitorCallback();
+  PubspecVisitor getPubspecVisitor() => visitorCallback();
 
   @override
   AstVisitor getVisitor() => visitorCallback();
@@ -617,7 +617,7 @@ class MockLintRule extends LintRule {
   AstVisitor getVisitor() => new MockVisitor(null);
 }
 
-class MockVisitor extends GeneralizingAstVisitor with PubSpecVisitor {
+class MockVisitor extends GeneralizingAstVisitor with PubspecVisitor {
   final nodeVisitor;
 
   MockVisitor(this.nodeVisitor);

diff --git a/test/mocks.dart b/test/mocks.dart
@@ -84,7 +84,7 @@ class MockLinterOptions extends Mock implements LinterOptions {
   noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
 }
 
-class MockPubVisitor extends Mock implements PubSpecVisitor {
+class MockPubVisitor extends Mock implements PubspecVisitor {
   @override
   noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
 }

diff --git a/test/pub_test.dart b/test/pub_test.dart
@@ -50,7 +50,7 @@ dev_dependencies:
   unittest: '>=0.11.0 <0.12.0'
 """;
 
-  PubSpec ps = new PubSpec.parse(src);
+  Pubspec ps = new Pubspec.parse(src);
 
   group('pubspec', () {
     group('basic', () {
@@ -87,9 +87,8 @@ dev_dependencies:
       testDepListContains(
           'dependencies', ps.dependencies, [{'analyzer': '0.24.0-dev.1'}]);
 
-      testDepListContains('dev_dependencies', ps.devDependencies, [
-        {'markdown': '>=0.7.1+2 <0.8.0'}
-      ]);
+      testDepListContains('dev_dependencies', ps.devDependencies,
+          [{'markdown': '>=0.7.1+2 <0.8.0'}]);
 
       group('hosted', () {
         PSDependency dep =
@@ -129,12 +128,21 @@ dev_dependencies:
     group('initialization', () {
       test('sourceUrl', () {
         File ps = new File('test/_data/p1/_pubspec.yaml');
-        PubSpec spec =
-            new PubSpec.parse(ps.readAsStringSync(), sourceUrl: ps.path);
+        Pubspec spec =
+            new Pubspec.parse(ps.readAsStringSync(), sourceUrl: ps.path);
         expect(spec.name.key.span.sourceUrl.toFilePath(),
             equals('test/_data/p1/_pubspec.yaml'));
       });
     });
+    group('parsing', () {
+      test('bad yaml', () {
+        File ps = new File('test/_data/p3/_pubspec.yaml');
+        Pubspec spec =
+            new Pubspec.parse(ps.readAsStringSync(), sourceUrl: ps.path);
+        expect(spec.name, isNull);
+        expect(spec.description, isNull);
+      });
+    });
   });
 }