Refactorings for TranslateUri and its tests.

pkg/front_end/lib/src/fasta/translate_uri.dart

Index: pkg/front_end/lib/src/fasta/translate_uri.dart
diff --git a/pkg/front_end/lib/src/fasta/translate_uri.dart b/pkg/front_end/lib/src/fasta/translate_uri.dart
index 2f0e3b0c1fcb6ec133d079824f11626fe3fbb0da..54dc024076e53d50ad8e490bfd446a88f29c806b 100644
--- a/pkg/front_end/lib/src/fasta/translate_uri.dart
+++ b/pkg/front_end/lib/src/fasta/translate_uri.dart
@@ -5,33 +5,56 @@
 library fasta.translate_uri;
 
 import 'dart:async' show Future;
-
 import 'dart:convert' show JSON;
 
 import 'package:front_end/file_system.dart'
     show FileSystem, FileSystemException;
-
 import 'package:package_config/packages_file.dart' as packages_file show parse;
 
 import 'deprecated_problems.dart' show deprecated_inputError;
 
+/// Instances of [TranslateUri] translate absolute `dart` and `package` URIs
+/// into corresponding file URIs in a [FileSystem]. Translated URIs are
+/// typically `file:` URIs, but may use a different scheme if the compiler is
+/// invoked with the `multiRoot` option or in unit tests that use a custom file
+/// system.
 class TranslateUri {
-  final Map<String, Uri> packages;
+  /// Mapping from Dart library names (e.g. `math`) to file URIs.
   final Map<String, Uri> dartLibraries;
 
   // TODO(ahe): We probably want this to be `Map<String, Uri>`, that is, just
   // one patch library (with parts).
-  final Map<String, List<Uri>> patches;
+  /// Mapping from Dart library names to the file URIs of patches to apply.
+  final Map<String, List<Uri>> dartPatches;

[ahe, 2017/07/12 09:16:27]

Why is this renamed?

[scheglov, 2017/07/12 14:34:55]

It makes the name consistent with [dartLibraries] and highlights that these are
patches for Dart libraries, not for packages.

[ahe, 2017/07/12 15:16:47]

But wouldn't "platform" be more accurate here? One can argue that any library
defined by a Dart source file is a Dart library. Also, can we know that all the
platform libraries use the "dart" scheme?

So it should be platformLibraryPatches.

But it's files, so should be platformLibraryPatchFiles or
platformLibraryDartSourceFilePatches?

I prefer terse names over long names that doesn't add anything. In this context
"dart" has zero value.

+
+  /// Mapping from package names (e.g. `angular`) to the file URIs.
+  final Map<String, Uri> packages;

[ahe, 2017/07/12 09:16:28]

Why is this moved?

[scheglov, 2017/07/12 14:34:55]

The order highlights the relative order and stability of information - at first
we know SDK, and then we add packages; or SDK packages change more often than
SDK.

[ahe, 2017/07/12 15:16:47]

I actually put packages first because I felt it was the most important parameter
(and the other parameters could be optional).

We could go about an move parameters around each time we read code, or we could
let it be. I prefer not moving code around unless there's a strong reason for
doing so. Not because you and I disagree what is most important.

 
-  TranslateUri(this.packages, this.dartLibraries, this.patches);
+  TranslateUri(this.dartLibraries, this.dartPatches, this.packages);

[ahe, 2017/07/12 09:16:27]

Why are the arguments rearranged on this constructor?

[scheglov, 2017/07/12 14:34:55]

This makes order of arguments consistent with the order of fields, and the
relative dependencies between these pieces of information.

+
+  /// Returns `true` if [uri] is private to the platform libraries (and thus
+  /// not accessible from user code).
+  bool isPlatformImplementation(Uri uri) {

[ahe, 2017/07/12 09:16:28]

Why is this moved?

[scheglov, 2017/07/12 14:34:55]

It sorts before "translate".
And it seems that it can be removed altogether - it is not used.

[ahe, 2017/07/12 15:16:48]

We haven't agreed that methods should be sorted by name. That's your subjective
stylistic choice that makes it harder for me to use a tool that is important to
me, "git blame".

+    if (uri.scheme != "dart") return false;
+    String path = uri.path;
+    return dartLibraries[path] == null || path.startsWith("_");
+  }
 
+  /// If the given [uri] is a `dart` or `package` URI, return the corresponding
+  /// file URI (possibly `null` if there is no corresponding file URI);
+  /// otherwise (e.g. when the [uri] is already a file URI) return `null`.
+  ///
+  /// This is the URIs only transformation, there is no guarantee that the
+  /// corresponding file exists in the file system.
   Uri translate(Uri uri) {
-    if (uri.scheme == "dart") return translateDartUri(uri);
-    if (uri.scheme == "package") return translatePackageUri(uri);
+    if (uri.scheme == "dart") return _translateDartUri(uri);
+    if (uri.scheme == "package") return _translatePackageUri(uri);
     return null;
   }
 
-  Uri translateDartUri(Uri uri) {
+  /// Return the file URI that corresponds to the given `dart` URI, or `null`
+  /// if there is no corresponding Dart library registered.
+  Uri _translateDartUri(Uri uri) {

[ahe, 2017/07/12 09:16:27]

Why is this made private?

[scheglov, 2017/07/12 14:34:55]

It is not used outside, and this is leaking implementation details that
TranslateUri supports only dart and package resolution. When I will implement
this interface for AnalysisDriver that uses KernelDriver, I need to have a nice
tight API.

[ahe, 2017/07/12 15:16:47]

This API isn't public, so everything is essentially private. You and I disagree
on when one should use privacy, but the solution to that disagreement is not to
be passive aggressive about it and make changes every time you read code I've
written.

     if (!uri.isScheme('dart')) return null;
     String path = uri.path;
 
@@ -44,7 +67,10 @@ class TranslateUri {
     return libraryFileUri?.resolve(relativePath);
   }
 
-  Uri translatePackageUri(Uri uri) {
+  /// Return the file URI that corresponds to the given `package` URI, or
+  /// `null` if the `package` [uri] format is invalid, or there is no
+  /// corresponding package registered.
+  Uri _translatePackageUri(Uri uri) {

[ahe, 2017/07/12 09:16:28]

Why is this made private?

[scheglov, 2017/07/12 14:34:55]

It is not used outside, and this is leaking implementation details that
TranslateUri supports only dart and package resolution. When I will implement
this interface for AnalysisDriver that uses KernelDriver, I need to have a nice
tight API.

     int index = uri.path.indexOf("/");
     if (index == -1) return null;
     String name = uri.path.substring(0, index);
@@ -54,14 +80,6 @@ class TranslateUri {
     return root.resolve(path);
   }
 
-  /// Returns true if [uri] is private to the platform libraries (and thus not
-  /// accessible from user code).
-  bool isPlatformImplementation(Uri uri) {
-    if (uri.scheme != "dart") return false;
-    String path = uri.path;
-    return dartLibraries[path] == null || path.startsWith("_");
-  }
-
   static Future<TranslateUri> parse(FileSystem fileSystem, Uri sdk,
       {Uri packages}) async {
     Uri librariesJson = sdk?.resolve("lib/libraries.json");
@@ -84,14 +102,17 @@ class TranslateUri {
     } on FormatException catch (e) {
       return deprecated_inputError(packages, e.offset, e.message);
     }
-    return new TranslateUri(
-        parsedPackages,
-        await computeLibraries(fileSystem, librariesJson),
-        await computePatches(fileSystem, patches));
+
+    var dartLibraries = await computeDartLibraries(fileSystem, librariesJson);
+    return new TranslateUri(dartLibraries,
+        await computeDartPatches(fileSystem, patches), parsedPackages);
   }
 }
 
-Future<Map<String, Uri>> computeLibraries(
+/// Read the JSON file with defined SDK libraries from the given [uri] in the
+/// [fileSystem] and return the mapping from parsed Dart library names (e.g.
+/// `math`) to file URIs.
+Future<Map<String, Uri>> computeDartLibraries(

[ahe, 2017/07/12 09:16:27]

Why is this renamed?

[scheglov, 2017/07/12 14:34:55]

This highlights the fact that we parse definitions of dart libraries, not just
any libraries.

[ahe, 2017/07/12 15:16:47]

We parse definitions of platform libraries, and there's no requirement that they
have the "dart" scheme. Also, "dart" adds no value in this context. It could
just as well be understood to be libraries written in Dart source code.

     FileSystem fileSystem, Uri uri) async {
   if (uri == null) return const <String, Uri>{};
   Map<String, String> libraries = JSON
@@ -103,7 +124,7 @@ Future<Map<String, Uri>> computeLibraries(
   return result;
 }
 
-Future<Map<String, List<Uri>>> computePatches(
+Future<Map<String, List<Uri>>> computeDartPatches(

[ahe, 2017/07/12 09:16:27]

Why is this renamed?

[scheglov, 2017/07/12 14:34:55]

The new name highlights that we parse patches for SDK libraries, not patches for
any libraries for packages or user code.

     FileSystem fileSystem, Uri uri) async {
   // TODO(ahe): Read patch information.
   return const <String, List<Uri>>{};