Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23249004:
Remove dependency on the Path class from dart:io (Closed)
|
Created:
7 years, 4 months ago by Søren Gjesse Modified:
7 years, 3 months ago CC:
reviews_dartlang.org Base URL:
https://github.com/dart-lang/dart-protoc-plugin.git@master Visibility:
Public. |
DescriptionRemove dependency on the Path class from dart:io
The Path class was removed from dart:io in Dart r26191. With this
change URIs are used to manipulate paths instead.
R=ajohnsen@google.com
BUG=
Committed: https://github.com/dart-lang/protoc-plugin/commit/a39b073
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed review comments #
Total comments: 11
Messages
Total messages: 9 (0 generated)
Hi Anders, Could you take a look (Anton is OOO)? I will move the relative code in a more generalized version to the Uri class later.
lgtm https://codereview.chromium.org/23249004/diff/1/lib/file_generator.dart File lib/file_generator.dart (right): https://codereview.chromium.org/23249004/diff/1/lib/file_generator.dart#newco... lib/file_generator.dart:58: List<String> pathSegments = target.pathSegments; pathSegments.sublist(0, pathSegments.length - 1) and remove baseLength? https://codereview.chromium.org/23249004/diff/1/lib/file_generator.dart#newco... lib/file_generator.dart:58: List<String> pathSegments = target.pathSegments; targetSegments? https://codereview.chromium.org/23249004/diff/1/lib/file_generator.dart#newco... lib/file_generator.dart:68: while (common < length && pathSegments[common] == baseSegments[common]) { This might 'fail' on Windows (lower/upper case stuff), but as the fallback is just to navigate to the root and then into target, it should be fine.
Thanks https://codereview.chromium.org/23249004/diff/1/lib/file_generator.dart File lib/file_generator.dart (right): https://codereview.chromium.org/23249004/diff/1/lib/file_generator.dart#newco... lib/file_generator.dart:58: List<String> pathSegments = target.pathSegments; On 2013/08/19 07:17:47, Anders Johnsen wrote: > pathSegments.sublist(0, pathSegments.length - 1) and remove baseLength? Done. https://codereview.chromium.org/23249004/diff/1/lib/file_generator.dart#newco... lib/file_generator.dart:58: List<String> pathSegments = target.pathSegments; On 2013/08/19 07:17:47, Anders Johnsen wrote: > targetSegments? Done. https://codereview.chromium.org/23249004/diff/1/lib/file_generator.dart#newco... lib/file_generator.dart:68: while (common < length && pathSegments[common] == baseSegments[common]) { On 2013/08/19 07:17:47, Anders Johnsen wrote: > This might 'fail' on Windows (lower/upper case stuff), but as the fallback is > just to navigate to the root and then into target, it should be fine. True, when we add this to the Uri class we should document this.
Message was sent while issue was closed.
Committed patchset #2 manually as ra39b073 (presubmit successful).
Message was sent while issue was closed.
lgtm, just have a couple questions/comments which I'm not sure about. https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... File lib/file_generator.dart (right): https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:54: Uri _relative(Uri target, Uri base) { could we instead depend on the path package and use the relative implementation there? https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:128: Uri relativeProtoPath = _relative(importPath, filePath); What do imports look like in the descriptor? Are they only paths or can they be URI themselves? In particular, if they could be 'package:foo/bar/baz' imports, we might be restricted to only use _relative if the import and the base path are on the same package.
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... File lib/file_generator.dart (right): https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:54: Uri _relative(Uri target, Uri base) { On 2013/08/19 16:44:48, Siggi Cherem (dart-lang) wrote: > could we instead depend on the path package and use the relative implementation > there? I would rather not rely on other packages in the protocol buffer compiler. We will add relative (or relativeTo) to the Uri class in dart:core, and then we can get rid of this again. https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:128: Uri relativeProtoPath = _relative(importPath, filePath); On 2013/08/19 16:44:48, Siggi Cherem (dart-lang) wrote: > What do imports look like in the descriptor? Are they only paths or can they be > URI themselves? > > In particular, if they could be 'package:foo/bar/baz' imports, we might be > restricted to only use _relative if the import and the base path are on the same > package. The imports in the descriptor are the literal strings (unquoted) from the import directive in the .proto file. What I found was that these have to be relative, and are searched for relative to the current directory and relative to the directories set with the -I option given to protoc. So they are only paths as protoc need to be able to find and open them as well.
Message was sent while issue was closed.
thanks! https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... File lib/file_generator.dart (right): https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:54: Uri _relative(Uri target, Uri base) { On 2013/08/20 07:35:33, Søren Gjesse wrote: > On 2013/08/19 16:44:48, Siggi Cherem (dart-lang) wrote: > > could we instead depend on the path package and use the relative > implementation > > there? > > I would rather not rely on other packages in the protocol buffer compiler. We > will add relative (or relativeTo) to the Uri class in dart:core, and then we can > get rid of this again. Sounds good, especially since this should be going away. Consider maybe adding a TODO about it. In general, I think it's totally ok to have extra dependencies in this package. If this is distributed as a pub package, then doing pub install will automatically include the dependencies we need. https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:128: Uri relativeProtoPath = _relative(importPath, filePath); On 2013/08/20 07:35:33, Søren Gjesse wrote: > On 2013/08/19 16:44:48, Siggi Cherem (dart-lang) wrote: > > What do imports look like in the descriptor? Are they only paths or can they > be > > URI themselves? > > > > In particular, if they could be 'package:foo/bar/baz' imports, we might be > > restricted to only use _relative if the import and the base path are on the > same > > package. > > The imports in the descriptor are the literal strings (unquoted) from the import > directive in the .proto file. What I found was that these have to be relative, > and are searched for relative to the current directory and relative to the > directories set with the -I option given to protoc. So they are only paths as > protoc need to be able to find and open them as well. I see, thanks. In the future we might want to start looking into how this will integrate with pub packages, pub-serve and pub-deploy. For instance, in polymer and other code generation tools we've written, we started seeing that people want to refer to paths in other packages. One way we allow to do that is to use paths that include a 'packages/' folder. For example: 'packages/foo/bar.proto' is then resolved as the lib/bar.proto path inside the 'foo' package.
Message was sent while issue was closed.
Sorry for late response, I've been on vacation. Mostly minor issues, my main concern is with path normalization part, but that'll be addressed in the lib if I understand it correctly, won't it? https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... File lib/file_generator.dart (right): https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:45: var dartFileName = _fileNameWithoutExtension(protoFilePath) + ".pb.dart"; nit: mix of ' and " https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:45: var dartFileName = _fileNameWithoutExtension(protoFilePath) + ".pb.dart"; nit: I'd rather use interpolation, but completely up to you https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:56: List<String> baseSegments = absolutely up to you, but I rather use new List<String>.from(base.pathSegments)..removeLast() https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:59: if (baseSegments.length == 1 && baseSegments[0] == '.') { what if baseSegments is [ '.', '.', '.' ] or alike? overall, is path normalization in this code good enough? https://chromiumcodereview.appspot.com/23249004/diff/8001/lib/file_generator.... lib/file_generator.dart:125: throw("FAILURE: Import with absolute path is not supported"); nit: mix of ' and " |
