Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2977703002:
Refactorings for TranslateUri and its tests. (Closed)
|
Created:
3 years, 5 months ago by scheglov Modified:
3 years, 5 months ago CC:
reviews_dartlang.org, dart-fe-team+reviews_google.com Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionRefactorings for TranslateUri and its tests.
R=ahe@google.com, paulberry@google.com, sigmund@google.com
BUG=
Committed: https://github.com/dart-lang/sdk/commit/cb3a79f8836509012c1d9bcbc7037b23d5f28dd2
Patch Set 1 #
Total comments: 4
Patch Set 2 : Revert including TODO into documentation comment. #
Total comments: 2
Patch Set 3 : tweak #Patch Set 4 : Replace `file` with just word 'file' in documentation. #
Total comments: 21
Messages
Total messages: 16 (1 generated)
Why is a refactoring needed?
lgtm, but I'm not terribly familiar with this code so you should probably get approval from one of the other reviewers as well. I am also curious why a refactoring is needed--perhaps add some text to the commit description indicating the motivation for this change.
On 2017/07/11 19:24:13, ahe wrote: > Why is a refactoring needed? KernelDriver (to be designed and implemented) will be used by AnalysisDriver to compute kernel files, and resynthesize Analyzer element models from it. One of the APIs that Fasta, so KernelDriver needs is TranslateUri. So, I need to make its API tighter to avoid implementing features nobody uses.
dbc - I'll follow up with more details on the email thread. https://codereview.chromium.org/2977703002/diff/1/pkg/front_end/lib/src/fasta... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/translate_uri.dart:23: /// TODO(ahe): We probably want this to be `Map<String, Uri>`, that is, just nit: remove one "/" so we keep TODOs as local comment and not part of the public dartdoc
PTAL Whatever we decide about additional improvements of TranslateUri, these changes are IMHO a step into correct direction. https://codereview.chromium.org/2977703002/diff/1/pkg/front_end/lib/src/fasta... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/translate_uri.dart:23: /// TODO(ahe): We probably want this to be `Map<String, Uri>`, that is, just On 2017/07/11 20:38:44, Siggi Cherem (dart-lang) wrote: > nit: remove one "/" so we keep TODOs as local comment and not part of the public > dartdoc I did this, but there are reasons to keep TODOs in documentation comments. For example in this case, this is a warning for the clients that API might change. And it's not like we have customers that might be annoyed when documentation includes more details than they would like to know.
lgtm, I generally like the new dartdocs. I don't feel moving up isPlatformImplementation is necessary though, but that might go away if we decide to delete it. Because Peter feels so strongly about private members, I think you should wait to hear from him before landing. https://codereview.chromium.org/2977703002/diff/1/pkg/front_end/lib/src/fasta... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/translate_uri.dart:40: /// If the given [uri] is a `file` or `package` URI, return the corresponding `file` => `dart`
https://codereview.chromium.org/2977703002/diff/1/pkg/front_end/lib/src/fasta... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/1/pkg/front_end/lib/src/fasta... pkg/front_end/lib/src/fasta/translate_uri.dart:40: /// If the given [uri] is a `file` or `package` URI, return the corresponding On 2017/07/11 21:41:29, Siggi Cherem (dart-lang) wrote: > `file` => `dart` Done.
https://codereview.chromium.org/2977703002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:17: /// into corresponding `file` URIs. Just looking at some other CL made me think that I'm not sure if can say that "translated" Uri is guaranteed to be a `file:` Uri. For example, with the multi-root option and when using custom-uris in unit tests, we might get a non file:* uri. One option is to say: Instances of [TranslateUri] translate absolute `dart` and `package` URIs into corresponding file-system Uris". The translated Uri is typically a `file:` Uri, 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.
https://codereview.chromium.org/2977703002/diff/20001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/20001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:17: /// into corresponding `file` URIs. On 2017/07/11 21:56:02, Siggi Cherem (dart-lang) wrote: > Just looking at some other CL made me think that I'm not sure if can say that > "translated" Uri is guaranteed to be a `file:` Uri. > > For example, with the multi-root option and when using custom-uris in unit > tests, we might get a non file:* uri. > > One option is to say: > > Instances of [TranslateUri] translate absolute `dart` and `package` URIs into > corresponding file-system Uris". The translated Uri is typically a `file:` Uri, > 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. I replaced `file` with just "file" to underscore that it is not literally file:// URIs.
I'm sorry, but I don't see the point of this CL. I think it's great that you add documentation, but as far as I'm concerned all the renaming and moving code around is unnecessary and will only serve to make using "git blame" harder to use. On principle, I'm against unnecessary renames and reordering of methods. FWIW, I often find that I have to ignore documentation as it gets stale, and the best resource I know for figuring out code does (besides reading it), is "git blame". This means that anything that makes "git blame" harder to use has a direct and negative impact on my productivity. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:28: final Map<String, List<Uri>> dartPatches; Why is this renamed? https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:31: final Map<String, Uri> packages; Why is this moved? https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:33: TranslateUri(this.dartLibraries, this.dartPatches, this.packages); Why are the arguments rearranged on this constructor? https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:37: bool isPlatformImplementation(Uri uri) { Why is this moved? https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:57: Uri _translateDartUri(Uri uri) { Why is this made private? https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:73: Uri _translatePackageUri(Uri uri) { Why is this made private? https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:115: Future<Map<String, Uri>> computeDartLibraries( Why is this renamed? https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:127: Future<Map<String, List<Uri>>> computeDartPatches( Why is this renamed?
Peter, this is not a serious approach. Holding back changes that make code better and allow future development because it was initially done in some way stifles innovation. How about using documentation and tests instead of code archeology? Yeah, I see that many people on this project don't like documentation and unit tests. Good luck next time in trying to makes sense of someone else big project. There is nothing precious in any code. Code should be sane and support project development, and not to become a holy cow just because it lived a couple of day, month, years. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:28: final Map<String, List<Uri>> dartPatches; On 2017/07/12 09:16:27, ahe wrote: > Why is this renamed? It makes the name consistent with [dartLibraries] and highlights that these are patches for Dart libraries, not for packages. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:31: final Map<String, Uri> packages; On 2017/07/12 09:16:28, ahe wrote: > Why is this moved? 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. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:33: TranslateUri(this.dartLibraries, this.dartPatches, this.packages); On 2017/07/12 09:16:27, ahe wrote: > Why are the arguments rearranged on this constructor? This makes order of arguments consistent with the order of fields, and the relative dependencies between these pieces of information. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:37: bool isPlatformImplementation(Uri uri) { On 2017/07/12 09:16:28, ahe wrote: > Why is this moved? It sorts before "translate". And it seems that it can be removed altogether - it is not used. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:57: Uri _translateDartUri(Uri uri) { On 2017/07/12 09:16:27, ahe wrote: > Why is this made private? 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. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:73: Uri _translatePackageUri(Uri uri) { On 2017/07/12 09:16:28, ahe wrote: > Why is this made private? 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. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:115: Future<Map<String, Uri>> computeDartLibraries( On 2017/07/12 09:16:27, ahe wrote: > Why is this renamed? This highlights the fact that we parse definitions of dart libraries, not just any libraries. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:127: Future<Map<String, List<Uri>>> computeDartPatches( On 2017/07/12 09:16:27, ahe wrote: > Why is this renamed? The new name highlights that we parse patches for SDK libraries, not patches for any libraries for packages or user code.
On 2017/07/12 14:34:55, scheglov wrote: > Peter, this is not a serious approach. I'm very serious. I'm not objecting to this because I have an attachment to code I've written. Each time code is moved around it impacts my productivity. Seriously. > Holding back changes that make code better and allow future development because > it was initially done in some way stifles innovation. I don't see how this change makes code better or is enabling future development. You say "make code better", but you haven't actually changed the code. You've just moved it around and made a few methods private. > How about using > documentation and tests instead of code archeology? I don't see these things as being mutually exclusive. If you have some concrete feedback you want to give me about lack of documentation, I have previously asked you to speak up, but I also thought we agreed that we had moved past that you bring it up every time we disagree about something. > Yeah, I see that many people on this project don't like documentation and unit > tests. Good luck next time in trying to makes sense of someone else big project. I think that's a specific reference to something you disagree with me about. When it comes to documentation, I have a minimalistic approach that I've developed over several years of working on large code bases that I didn't write from scratch. I rarely read documentation because when I do, I find the documentation to be out of date, or lacking information about the specific details I need. I've noticed that many colleagues don't update documentation when changing the implementation. However, I thought we had already agreed to disagree on that point and established that if you have specific concerns, I'll try to update the documentation. > There is nothing precious in any code. Code should be sane and support project > development, and not to become a holy cow just because it lived a couple of day, > month, years. I agree that there's code isn't precious. But that doesn't mean that you should arbitrarily rename and move stuff around unless you have a good reason. And a good reason isn't: members should be sorted alphabetically. That is a subjective style preference that you have, but one that we haven't discussed and reached an agreement on, and one which I have brought up several times because it makes it harder for me to use "git blame".
https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... File pkg/front_end/lib/src/fasta/translate_uri.dart (right): https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:28: final Map<String, List<Uri>> dartPatches; On 2017/07/12 14:34:55, scheglov wrote: > On 2017/07/12 09:16:27, ahe wrote: > > Why is this renamed? > > It makes the name consistent with [dartLibraries] and highlights that these are > patches for Dart libraries, not for packages. 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. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:31: final Map<String, Uri> packages; On 2017/07/12 14:34:55, scheglov wrote: > On 2017/07/12 09:16:28, ahe wrote: > > Why is this moved? > > 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. 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. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:37: bool isPlatformImplementation(Uri uri) { On 2017/07/12 14:34:55, scheglov wrote: > On 2017/07/12 09:16:28, ahe wrote: > > Why is this moved? > > It sorts before "translate". > And it seems that it can be removed altogether - it is not used. 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". https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:57: Uri _translateDartUri(Uri uri) { On 2017/07/12 14:34:55, scheglov wrote: > On 2017/07/12 09:16:27, ahe wrote: > > Why is this made private? > > 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. 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. https://codereview.chromium.org/2977703002/diff/60001/pkg/front_end/lib/src/f... pkg/front_end/lib/src/fasta/translate_uri.dart:115: Future<Map<String, Uri>> computeDartLibraries( On 2017/07/12 14:34:55, scheglov wrote: > On 2017/07/12 09:16:27, ahe wrote: > > Why is this renamed? > > This highlights the fact that we parse definitions of dart libraries, not just > any libraries. 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.
Description was changed from ========== Refactorings for TranslateUri and its tests. R=ahe@google.com, paulberry@google.com, sigmund@google.com BUG= ========== to ========== Refactorings for TranslateUri and its tests. R=ahe@google.com, paulberry@google.com, sigmund@google.com BUG= Committed: https://github.com/dart-lang/sdk/commit/cb3a79f8836509012c1d9bcbc7037b23d5f28dd2 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as cb3a79f8836509012c1d9bcbc7037b23d5f28dd2 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
