Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10417053:
Add Path class to dart:io, and add unit tests for it. (Closed)
|
Created:
8 years, 7 months ago by Bill Hesse Modified:
8 years, 6 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd Path class to dart:io, add tests for it to standalone/io.
Just a preliminary implementation - make suggestions for changes.
BUG=
TEST=standalone/io/path_test
Committed: https://code.google.com/p/dart/source/detail?r=8909
Patch Set 1 #Patch Set 2 : more changes. #Patch Set 3 : most recent version #Patch Set 4 : Add tests, remove test_suite.dart changes. #Patch Set 5 : Add unit tests for Path, remove test_suite changes. #
Total comments: 71
Patch Set 6 : Address comments, finish implementation (mostly). #
Total comments: 44
Patch Set 7 : Address comments. #
Total comments: 30
Patch Set 8 : Address comments. Add to docgen. #Patch Set 9 : Fix 2 bugs. #
Messages
Total messages: 14 (0 generated)
Just a preliminary stab at it. Please make suggestions for changes. One design decision that is separable from the rest is: Do we clean the path on input, or does canonicalize do all path cleaning? Does this include the backslash conversion on Windows? http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart File runtime/bin/path_impl.dart (right): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:51: throw "Unimplemented case ofPath.relativeTo(base):" Switch to throw PathException (or UnimplementedException?) http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:56: if (further.isAbsolute) { PathException.
First round of comments. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart File runtime/bin/path.dart (right): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newcode9 runtime/bin/path.dart:9: Path(String source); I think we should have - const Path(source) - Path.join(List parts); http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:14: bool get isDirectory(); Since it have no knowledge of the underlying system, we can not answer this question. I think we should remove it, and have something more explicit being a test for tailing '/'. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:18: Path join(further); // further is Path or String. Comment in /** ... **/ so we can mark it with as [further]. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:19: Path relativeTo(Path base); I'm in favor of relativePathTo, or pathRelativeTo, since relativeTo could sound like a boolean question. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:29: Path dirname(); // or get directoryPath IMO, dirname != directoryPath. E.g.: /my/path/to/a/dir dirname == dir (same as filename iff no tailing '/') directoryPath == /my/path/to/a/
http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart File runtime/bin/path.dart (right): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newcode8 runtime/bin/path.dart:8: interface Path default _PathImpl { Each constructor/method should have a documentation comment. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:10: const Path.c(String source); What does the Path.c constructor do? http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:28: // foo.extension() is nonempty. I am in favor of these being getters. Regarding the naming I think they should be short. How about having first(int n) and last(int n) methods methods as well. With negative indexes these could take from the other end, e.g last(-1) will return a path with all but the last element. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:29: Path dirname(); // or get directoryPath On 2012/05/25 13:37:30, ajohnsen wrote: > IMO, dirname != directoryPath. > E.g.: /my/path/to/a/dir > dirname == dir (same as filename iff no tailing '/') > directoryPath == /my/path/to/a/ I agree with Anders that we should use "directory" with care. The path itself cannot know whether it is dealing with a directory or a file. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:32: String extension(); // or get extension How about mentioning toString and document what it does. Should there be other toString variants e.g toPlatformString and toShortString? http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart File runtime/bin/path_impl.dart (right): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:23: if (const RegExp(@'^[a-zA-Z]:').hasMatch(clean)) { Just if (clean.length > 1 && clean[1] = ":") instead of the RegExp. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:110: return path.substring(pos+1); Spaces on both sides of +. http://codereview.chromium.org/10417053/diff/7001/tests/standalone/io/path_te... File tests/standalone/io/path_test.dart (right): http://codereview.chromium.org/10417053/diff/7001/tests/standalone/io/path_te... tests/standalone/io/path_test.dart:11: // Remove these two lines when committing. If you are only unit-testing the path library you can just keep these two #source lines and remove the #import. This way you can also test stuff in _Path that is not public.
I think the interface looks like a good start. Let's get this fully implemented! :) http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart File runtime/bin/path.dart (right): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newcode9 runtime/bin/path.dart:9: Path(String source); On 2012/05/25 13:37:30, ajohnsen wrote: > I think we should have > - const Path(source) > - Path.join(List parts); I agree that it should be a const constructor. The join constructor could be nice if it doesn't conflict with the join method (need to check the spec). http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:18: Path join(further); // further is Path or String. On 2012/05/25 13:37:30, ajohnsen wrote: > Comment in /** ... **/ so we can mark it with as [further]. Yes, and please add doc comments to all of these since this is a public interface. Anders' comment about 'isDirectory' would be less of an issue if there is a clear explanation of the functionality. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:19: Path relativeTo(Path base); On 2012/05/25 13:37:30, ajohnsen wrote: > I'm in favor of relativePathTo, or pathRelativeTo, since relativeTo could sound > like a boolean question. On the other hand, all of the getters in the interface are called 'isSomething' so I'm not sure there will be much confusion. This method needs a good doc comment with a couple of examples (including one that will throw an exception). http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:26: // '$foo' == '${foo.dirname()}/${foo.filename()}' if dirname is nonempty. I don't understand these comments. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart File runtime/bin/path_impl.dart (right): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:8: const _PathImpl.c(String source) : path = source; const _PathImpl.c(String this.path); ? Where do you use this? http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:35: bool isEmpty() => path == ''; Add 'get' and either remove the blank line below or have a blank between the two other getters. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:51: throw "Unimplemented case ofPath.relativeTo(base):" On 2012/05/25 13:14:38, Bill Hesse wrote: > Switch to throw PathException (or UnimplementedException?) I would throw a PathException and be very clear about the rules in the documentation. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:60: // Canonicalize? Add TODO(whesse): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:63: Path safeJoin(Path further) => join(further); This is not in the interface. What is it used for? Delete? http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:77: components.removeRange(0, 1); I would use indices instead of copying 'components' repeatedly. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:92: throw "Unimplemented Path.makeCanonical()"; NotImplementedException, but it doesn't really matter since we should implement before landing. :-) http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:99: if (const RegExp(@'^/[a-zA-z]:').hasMatch(nativePath)) { I guess you could just check for '/' and maybe ':' here instead of using regexps. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:108: String last() { Make this private since it is not part of the interface. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:113: Path dropLast() { Move the code to dirname which is the only user. If you don't want to do that, make dropLast private. It is not part of the interface and therefore should not be callable by others. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:116: // while (pos > 0 && path[pos - 1] == '/') --pos; Code in comments. http://codereview.chromium.org/10417053/diff/7001/tools/test.dart File tools/test.dart (right): http://codereview.chromium.org/10417053/diff/7001/tools/test.dart#newcode34 tools/test.dart:34: #import("../samples/tests/dartc/test_config.dart"); // DartcCompilationTestS.. ? Move the comments to the lines above the imports so you don't have issues with line length. Also, these changes seem unrelated to this change so maybe just revert this file?
DBC. I would also recommend you run this by Josh. I would really like us to have a more thorough design review process for public APIs so that hopefully they will be more consistent with each other. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart File runtime/bin/path.dart (right): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newcode8 runtime/bin/path.dart:8: interface Path default _PathImpl { Why isn't Path a class? http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:19: Path relativeTo(Path base); On 2012/05/29 07:36:02, Mads Ager wrote: > On 2012/05/25 13:37:30, ajohnsen wrote: > > I'm in favor of relativePathTo, or pathRelativeTo, since relativeTo could > sound > > like a boolean question. > > On the other hand, all of the getters in the interface are called 'isSomething' > so I'm not sure there will be much confusion. This method needs a good doc > comment with a couple of examples (including one that will throw an exception). > +1 to Mads. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:28: // foo.extension() is nonempty. On 2012/05/29 07:18:01, Søren Gjesse wrote: > I am in favor of these being getters. Regarding the naming I think they should > be short. +1. These should be getters. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path.dart#newco... runtime/bin/path.dart:35: class PathException implements Exception { I don't think you should have this. We should only define new exception classes when they represent something that a user may want to catch differently. We shouldn't define an exception class that just tells you want class it was thrown from. The caller already knows that: they know what they're calling. http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart File runtime/bin/path_impl.dart (right): http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:38: bool get isDirectory() => path.endsWith('/') || isEmpty(); When you make isEmpty a getter, don't forget to remove the () here. :) http://codereview.chromium.org/10417053/diff/7001/runtime/bin/path_impl.dart#... runtime/bin/path_impl.dart:56: if (further.isAbsolute) { On 2012/05/25 13:14:38, Bill Hesse wrote: > PathException. Better: IllegalArgumentException.
https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.dart File runtime/bin/path.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:8: interface Path default _PathImpl { On 2012/05/30 17:58:37, Bob Nystrom wrote: > Why isn't Path a class? Only so that the interface is clear to the reader of path.dart. Is that not a good reason? https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:9: Path(String source); Path() is now a const constructor Path.fromNative() is a non-const constructor that only does changes on Windows (/ to \ and initial / before c:). Path.join(List) can be done later, if possible. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:14: bool get isDirectory(); Now called get hasTrailingSlash(). https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:18: Path join(further); // further is Path or String. On 2012/05/29 07:36:02, Mads Ager wrote: > On 2012/05/25 13:37:30, ajohnsen wrote: > > Comment in /** ... **/ so we can mark it with as [further]. > > Yes, and please add doc comments to all of these since this is a public > interface. Anders' comment about 'isDirectory' would be less of an issue if > there is a clear explanation of the functionality. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:19: Path relativeTo(Path base); On 2012/05/30 17:58:37, Bob Nystrom wrote: > On 2012/05/29 07:36:02, Mads Ager wrote: > > On 2012/05/25 13:37:30, ajohnsen wrote: > > > I'm in favor of relativePathTo, or pathRelativeTo, since relativeTo could > > sound > > > like a boolean question. > > > > On the other hand, all of the getters in the interface are called > 'isSomething' > > so I'm not sure there will be much confusion. This method needs a good doc > > comment with a couple of examples (including one that will throw an > exception). > > > > +1 to Mads. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:26: // '$foo' == '${foo.dirname()}/${foo.filename()}' if dirname is nonempty. On 2012/05/29 07:36:02, Mads Ager wrote: > I don't understand these comments. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:28: // foo.extension() is nonempty. On 2012/05/30 17:58:37, Bob Nystrom wrote: > On 2012/05/29 07:18:01, Søren Gjesse wrote: > > I am in favor of these being getters. Regarding the naming I think they should > > be short. > > +1. These should be getters. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:28: // foo.extension() is nonempty. On 2012/05/30 17:58:37, Bob Nystrom wrote: > On 2012/05/29 07:18:01, Søren Gjesse wrote: > > I am in favor of these being getters. Regarding the naming I think they should > > be short. > > +1. These should be getters. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:29: Path dirname(); // or get directoryPath On 2012/05/29 07:18:01, Søren Gjesse wrote: > On 2012/05/25 13:37:30, ajohnsen wrote: > > IMO, dirname != directoryPath. > > E.g.: /my/path/to/a/dir > > dirname == dir (same as filename iff no tailing '/') > > directoryPath == /my/path/to/a/ > > I agree with Anders that we should use "directory" with care. The path itself > cannot know whether it is dealing with a directory or a file. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:32: String extension(); // or get extension toNativePath is what I called toPlatformString, but I think toPlatformString is better. What would toShortString do? I put toString in the interface and documented it, but I think that is illegal Dart. On 2012/05/29 07:18:01, Søren Gjesse wrote: > How about mentioning toString and document what it does. > > Should there be other toString variants e.g toPlatformString and toShortString? https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:35: class PathException implements Exception { On 2012/05/30 17:58:37, Bob Nystrom wrote: > I don't think you should have this. We should only define new exception classes > when they represent something that a user may want to catch differently. We > shouldn't define an exception class that just tells you want class it was thrown > from. The caller already knows that: they know what they're calling. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... File runtime/bin/path_impl.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:8: const _PathImpl.c(String source) : path = source; On 2012/05/29 07:36:02, Mads Ager wrote: > const _PathImpl.c(String this.path); > ? > > Where do you use this? Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:23: if (const RegExp(@'^[a-zA-Z]:').hasMatch(clean)) { On 2012/05/29 07:18:01, Søren Gjesse wrote: > Just > > if (clean.length > 1 && clean[1] = ":") > > instead of the RegExp. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:35: bool isEmpty() => path == ''; On 2012/05/29 07:36:02, Mads Ager wrote: > Add 'get' and either remove the blank line below or have a blank between the two > other getters. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:38: bool get isDirectory() => path.endsWith('/') || isEmpty(); On 2012/05/30 17:58:37, Bob Nystrom wrote: > When you make isEmpty a getter, don't forget to remove the () here. :) Removed this case from hasTrailingSlash. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:51: throw "Unimplemented case ofPath.relativeTo(base):" The PathException class is removed. Throwing an UnimplementedException. On 2012/05/29 07:36:02, Mads Ager wrote: > On 2012/05/25 13:14:38, Bill Hesse wrote: > > Switch to throw PathException (or UnimplementedException?) > > I would throw a PathException and be very clear about the rules in the > documentation. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:56: if (further.isAbsolute) { On 2012/05/30 17:58:37, Bob Nystrom wrote: > On 2012/05/25 13:14:38, Bill Hesse wrote: > > PathException. > > Better: IllegalArgumentException. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:60: // Canonicalize? On 2012/05/29 07:36:02, Mads Ager wrote: > Add TODO(whesse): Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:63: Path safeJoin(Path further) => join(further); On 2012/05/29 07:36:02, Mads Ager wrote: > This is not in the interface. What is it used for? Delete? Added to the interface. It joins two paths, checking that the second does not include an upward traversal into the first. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:77: components.removeRange(0, 1); On 2012/05/29 07:36:02, Mads Ager wrote: > I would use indices instead of copying 'components' repeatedly. Yes, that would be better. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:77: components.removeRange(0, 1); On 2012/05/29 07:36:02, Mads Ager wrote: > I would use indices instead of copying 'components' repeatedly. Fixed using indices, but keeping the components.some check at the end. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:92: throw "Unimplemented Path.makeCanonical()"; On 2012/05/29 07:36:02, Mads Ager wrote: > NotImplementedException, but it doesn't really matter since we should implement > before landing. :-) Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:99: if (const RegExp(@'^/[a-zA-z]:').hasMatch(nativePath)) { On 2012/05/29 07:36:02, Mads Ager wrote: > I guess you could just check for '/' and maybe ':' here instead of using > regexps. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:99: if (const RegExp(@'^/[a-zA-z]:').hasMatch(nativePath)) { On 2012/05/29 07:36:02, Mads Ager wrote: > I guess you could just check for '/' and maybe ':' here instead of using > regexps. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:108: String last() { On 2012/05/29 07:36:02, Mads Ager wrote: > Make this private since it is not part of the interface. removed. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:110: return path.substring(pos+1); On 2012/05/29 07:18:01, Søren Gjesse wrote: > Spaces on both sides of +. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:110: return path.substring(pos+1); On 2012/05/29 07:18:01, Søren Gjesse wrote: > Spaces on both sides of +. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:113: Path dropLast() { On 2012/05/29 07:36:02, Mads Ager wrote: > Move the code to dirname which is the only user. If you don't want to do that, > make dropLast private. It is not part of the interface and therefore should not > be callable by others. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:113: Path dropLast() { On 2012/05/29 07:36:02, Mads Ager wrote: > Move the code to dirname which is the only user. If you don't want to do that, > make dropLast private. It is not part of the interface and therefore should not > be callable by others. Done. https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path_im... runtime/bin/path_impl.dart:116: // while (pos > 0 && path[pos - 1] == '/') --pos; On 2012/05/29 07:36:02, Mads Ager wrote: > Code in comments. Uncommented, because we don't always clean consecutive / marks now. https://chromiumcodereview.appspot.com/10417053/diff/7001/tests/standalone/io... File tests/standalone/io/path_test.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/7001/tests/standalone/io... tests/standalone/io/path_test.dart:11: // Remove these two lines when committing. On 2012/05/29 07:18:01, Søren Gjesse wrote: > If you are only unit-testing the path library you can just keep these two > #source lines and remove the #import. This way you can also test stuff in _Path > that is not public. I need to add a mock Platform in that case, unless I include dart:io, but then Path conflicts with the Path in dart:io. https://chromiumcodereview.appspot.com/10417053/diff/7001/tools/test.dart File tools/test.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/7001/tools/test.dart#new... tools/test.dart:34: #import("../samples/tests/dartc/test_config.dart"); // DartcCompilationTestS.. Removed from this commit. On 2012/05/29 07:36:02, Mads Ager wrote: > ? > > Move the comments to the lines above the imports so you don't have issues with > line length. > > Also, these changes seem unrelated to this change so maybe just revert this > file? https://chromiumcodereview.appspot.com/10417053/diff/7001/tools/test.dart#new... tools/test.dart:34: #import("../samples/tests/dartc/test_config.dart"); // DartcCompilationTestS.. On 2012/05/29 07:36:02, Mads Ager wrote: > ? > > Move the comments to the lines above the imports so you don't have issues with > line length. > > Also, these changes seem unrelated to this change so maybe just revert this > file? File reverted
https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.dart File runtime/bin/path.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... runtime/bin/path.dart:8: interface Path default _PathImpl { On 2012/05/31 15:55:10, Bill Hesse wrote: > On 2012/05/30 17:58:37, Bob Nystrom wrote: > > Why isn't Path a class? > > Only so that the interface is clear to the reader of path.dart. Is that not a > good reason? The interface is clear either way: they'll see a type with some members. Most users will learn the API through auto-complete in the editor or from api.dartlang.org before they ever venture into the code. Classes are, I believe a much simpler mental model than interfaces: they are a concrete thing you can construct. For someone coming from a dynamic language, interfaces are distinctly challenging since they don't exist outside of statically-typed languages and don't actually do anything useful outside of the type system. We get frequent feedback that default classes are really hard for users to understand. I think there may be some cases where it makes sense to use them, but if you have a conceptual "thing" that has a single implementation of some operations and can be constructed, there's already a perfectly natural mechanism for that: a class. Given that we have implicit interfaces, I think we should need a strong compelling reason before we reach for an interface. Otherwise, it's just adding needless complexity.
On 2012/05/31 18:06:46, Bob Nystrom wrote: > https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.dart > File runtime/bin/path.dart (right): > > https://chromiumcodereview.appspot.com/10417053/diff/7001/runtime/bin/path.da... > runtime/bin/path.dart:8: interface Path default _PathImpl { > On 2012/05/31 15:55:10, Bill Hesse wrote: > > On 2012/05/30 17:58:37, Bob Nystrom wrote: > > > Why isn't Path a class? > > > > Only so that the interface is clear to the reader of path.dart. Is that not a > > good reason? > > The interface is clear either way: they'll see a type with some members. Most > users will learn the API through auto-complete in the editor or from > http://api.dartlang.org before they ever venture into the code. > > Classes are, I believe a much simpler mental model than interfaces: they are a > concrete thing you can construct. For someone coming from a dynamic language, > interfaces are distinctly challenging since they don't exist outside of > statically-typed languages and don't actually do anything useful outside of the > type system. > > We get frequent feedback that default classes are really hard for users to > understand. I think there may be some cases where it makes sense to use them, > but if you have a conceptual "thing" that has a single implementation of some > operations and can be constructed, there's already a perfectly natural mechanism > for that: a class. > > Given that we have implicit interfaces, I think we should need a strong > compelling reason before we reach for an interface. Otherwise, it's just adding > needless complexity. I understand that default classes is an extra thing for users to understand. On the other hand I haven't heard any users of dart:io complaint that everything (almost) in dart:io is an interface with a default implementation. Since this code is going into dart:io using an interface with a default implementation is consistent with the rest. We can move to classes for all of dart:io later if needed.
Next round of comments. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.dart File runtime/bin/path.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:11: const Path(String source); Please document the constructor. Maybe with a comment about what happens when you create a Path with an invalid path string and operate on it? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:25: * Does this path begin with '/'? It seems strange that the getter name is 'isAbsolute' but the word absolute is not used in the comment at all. Seems like either the comment should use the word 'absolute' or the name should change to beginsWithSeparator (which would also make it consistent with endsWithSeparator below). https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:32: bool get hasTrailingSlash(); I think I would prefer 'path separator' instead of 'slash'. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:35: * Does this path contain no segments . or .., except leading .. segments, If you use the term segments here, maybe the initial comment about the path class should use the term segment as well: 'A Path, interpreted as a sequence of path segments (components?) separated by forward slashes.'? Maybe put quotes around '.', '..'? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:41: * Make a path canonical by dropping . segments, canceling .. segments with Ditto. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:48: * or relative. Canonicalizes the path, to remove ., .., and //. Examples or a more thorough explanation would be good. Does this take the components/segments of [further] and append them to the components/segments of 'this'. In that case it never fails? What is it that may be absolute or relative? Both [this] and [further]? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:57: * begin with '..' I think examples would be good here. What happens if you do something unsafe? Throws an exception? Could be good to document that as well. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:74: * Gets the segments of a Path (the strings separated by /) Instead of the parenthesis you could have an example instead. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:79: * Drops the final '/' and whatever follows it from this Path, and returns should we write out 'path separator' instead of '/' in these comments? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:81: * character, returns '/' instead of ''. If there is no '/' in the Path, Text is good. Examples is better. :) Also in the text I would write out 'empty string' instead of '' https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... File runtime/bin/path_impl.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:30: bool get isEmpty() => path == ''; bool get isEmpty() => path.isEmpty(); https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:36: Path relativeTo(Path base) { This is not in the interface. Remove for now and add later or add to the interface and document thoroughly. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:53: throw new IllegalArgumentException( This is not documented in the interface. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:84: // Contains no consecutive /s. /s -> path separators. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:88: List components = path.split('/'); segments and isAbsolute instead of split and check first component? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:90: components[0] = 'Okay'; WAT? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:95: components[pos] = 'Okay'; Ditto? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:98: if (components.isEmpty()) return true; ? You have just accessed components[0]. We need more tests. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:105: bool isAbs = isAbsolute; Why? Isn't 'isAbsolute' a fine name? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:106: List components = path.split('/'); path.split -> segments? https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:107: Expect.isNotNull(components); If this is needed here it probably is elsewhere too. I think you can safely remove this one. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:112: if (isAbs && Move this if inside the if above instead of repeating the isAbsolute check? https://chromiumcodereview.appspot.com/10417053/diff/3008/tests/standalone/io... File tests/standalone/io/path_test.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/3008/tests/standalone/io... tests/standalone/io/path_test.dart:49: Remove a couple of these empty lines?
The interface and class are not showing up in the generated API docs - I don't know what I need to do to get them processed. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.dart File runtime/bin/path.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:11: const Path(String source); On 2012/06/01 08:11:31, Mads Ager wrote: > Please document the constructor. Maybe with a comment about what happens when > you create a Path with an invalid path string and operate on it? Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:25: * Does this path begin with '/'? On 2012/06/01 08:11:31, Mads Ager wrote: > It seems strange that the getter name is 'isAbsolute' but the word absolute is > not used in the comment at all. Seems like either the comment should use the > word 'absolute' or the name should change to beginsWithSeparator (which would > also make it consistent with endsWithSeparator below). Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:32: bool get hasTrailingSlash(); On 2012/06/01 08:11:31, Mads Ager wrote: > I think I would prefer 'path separator' instead of 'slash'. Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:35: * Does this path contain no segments . or .., except leading .. segments, On 2012/06/01 08:11:31, Mads Ager wrote: > If you use the term segments here, maybe the initial comment about the path > class should use the term segment as well: 'A Path, interpreted as a sequence of > path segments (components?) separated by forward slashes.'? > > Maybe put quotes around '.', '..'? Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:41: * Make a path canonical by dropping . segments, canceling .. segments with On 2012/06/01 08:11:31, Mads Ager wrote: > Ditto. Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:48: * or relative. Canonicalizes the path, to remove ., .., and //. On 2012/06/01 08:11:31, Mads Ager wrote: > Examples or a more thorough explanation would be good. Does this take the > components/segments of [further] and append them to the components/segments of > 'this'. In that case it never fails? What is it that may be absolute or > relative? Both [this] and [further]? Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:57: * begin with '..' On 2012/06/01 08:11:31, Mads Ager wrote: > I think examples would be good here. What happens if you do something unsafe? > Throws an exception? Could be good to document that as well. Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:74: * Gets the segments of a Path (the strings separated by /) On 2012/06/01 08:11:31, Mads Ager wrote: > Instead of the parenthesis you could have an example instead. Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:79: * Drops the final '/' and whatever follows it from this Path, and returns On 2012/06/01 08:11:31, Mads Ager wrote: > should we write out 'path separator' instead of '/' in these comments? Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:81: * character, returns '/' instead of ''. If there is no '/' in the Path, On 2012/06/01 08:11:31, Mads Ager wrote: > Text is good. Examples is better. :) > > Also in the text I would write out 'empty string' instead of '' Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path.da... runtime/bin/path.dart:105: class PathException implements Exception { Removed. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... File runtime/bin/path_impl.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:84: // Contains no consecutive /s. On 2012/06/01 08:11:31, Mads Ager wrote: > /s -> path separators. Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:88: List components = path.split('/'); On 2012/06/01 08:11:31, Mads Ager wrote: > segments and isAbsolute instead of split and check first component? Yes, I was just optimizing. This avoids a removeRange in the segments getter. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:90: components[0] = 'Okay'; On 2012/06/01 08:11:31, Mads Ager wrote: > WAT? Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:95: components[pos] = 'Okay'; On 2012/06/01 08:11:31, Mads Ager wrote: > Ditto? Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:98: if (components.isEmpty()) return true; On 2012/06/01 08:11:31, Mads Ager wrote: > ? You have just accessed components[0]. We need more tests. Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:105: bool isAbs = isAbsolute; On 2012/06/01 08:11:31, Mads Ager wrote: > Why? Isn't 'isAbsolute' a fine name? I was just optimizing. It is used about 5 times. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:106: List components = path.split('/'); On 2012/06/01 08:11:31, Mads Ager wrote: > path.split -> segments? Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:107: Expect.isNotNull(components); On 2012/06/01 08:11:31, Mads Ager wrote: > If this is needed here it probably is elsewhere too. I think you can safely > remove this one. Done. https://chromiumcodereview.appspot.com/10417053/diff/3008/runtime/bin/path_im... runtime/bin/path_impl.dart:112: if (isAbs && On 2012/06/01 08:11:31, Mads Ager wrote: > Move this if inside the if above instead of repeating the isAbsolute check? This is only hit in the exact case it needs to be checked, and isAbs is already a boolean local. If we moved it above, we would need to remove them from the list, or keep a pointer into the list, and start the next loop at that pointer. There is complex processing in the '..' case of the switch in any case, and this fits naturally. https://chromiumcodereview.appspot.com/10417053/diff/3008/tests/standalone/io... File tests/standalone/io/path_test.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/3008/tests/standalone/io... tests/standalone/io/path_test.dart:49: On 2012/06/01 08:11:31, Mads Ager wrote: > Remove a couple of these empty lines? Done.
https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.dart File runtime/bin/path.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:54: Path canonicalize(); I'm not sure what I like the most, so what does other people feel about canonicalize vs. canonicalForm()? https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:88: * case is not implemented yet. A few comments here. 1) Do you really mean "if no such path exists"? I thought this lib was purely acting as a String manipulator? 2) What do you mean by a case is not implemented? If something is not implementable, we should write it here. And AFAIK, there is a few cases that'll not work out? https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:130: Path get directoryPath(); I really like this one! Thanks! https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:150: String get filenameWithoutExtension(); I see you went for this, and not basename. Given that basename conflicts with C, I think this might be the right thing to do. https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:159: String get extension(); With the classic case of .tar.gz, should we have a "fullExtension", or leave it to the use to extract that?
LGTM with comments addressed. https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.dart File runtime/bin/path.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:12: interface Path default _PathImpl { Following the convention for the rest of dart:io default class should be just _Path (without Impl) https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:33: * Is this path an absolute path, beginning with a path separator? Should there be some additional information on Windows and drive letters? How is a drive letter without a '/' before interpreted? https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:52: * and combining consecutive path separators. What happens if there are more '..'s that "real" segments? Exception? https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:58: * interpreting '.' and '..' as directory traversal commands, and removing The explanation of '.' and '..' here is slightly different that for canonicalize. Maybe just say that the raw joined path is canonicalizeed to avoid duplicating the description. https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:79: * `further.canonicalize()` starts with '../' or equals '..'. Is that sufficient? What about a [further] of 'x/../..'? https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:96: * the leading path separator if the path starts with a drive specification. Should the invariant here be that is the fromNative constructor was used that toNativePath should return the exact same string? https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:98: String toNativePath(); The name toNativePath indicate that a Path not a String is returned. https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:139: String get filename(); fileName (uppercase N)? https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path.d... runtime/bin/path.dart:150: String get filenameWithoutExtension(); fileNameWithoutExtension (uppercase N)? https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path_i... File runtime/bin/path_impl.dart (right): https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path_i... runtime/bin/path_impl.dart:22: var clean = source.replaceAll('\\', '/'); Maybe create static final fields for '/', '\\', ':', '.' and '..'. static final String SEPARATOR = '/'; ... https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path_i... runtime/bin/path_impl.dart:48: throw new Exception( We have UnsupportedOperationException and NotImplementedException - maybe use one of these instead. https://chromiumcodereview.appspot.com/10417053/diff/18001/runtime/bin/path_i... runtime/bin/path_impl.dart:173: List result = path.split('/'); Maybe cache the result of splitting?
To get the interfaces to show up in the generated documentation you have to add the sources to lib/compiler/implementation/lib/io.dart (and yes, that is very, very broken). :-)
http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart File runtime/bin/path.dart (right): http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:12: interface Path default _PathImpl { On 2012/06/18 07:29:35, Søren Gjesse wrote: > Following the convention for the rest of dart:io default class should be just > _Path (without Impl) Done. http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:33: * Is this path an absolute path, beginning with a path separator? On 2012/06/18 07:29:35, Søren Gjesse wrote: > Should there be some additional information on Windows and drive letters? How is > a drive letter without a '/' before interpreted? Done. http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:52: * and combining consecutive path separators. On 2012/06/18 07:29:35, Søren Gjesse wrote: > What happens if there are more '..'s that "real" segments? Exception? Leading '..' segments are kept on relative paths, and dropped on absolute paths. http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:58: * interpreting '.' and '..' as directory traversal commands, and removing On 2012/06/18 07:29:35, Søren Gjesse wrote: > The explanation of '.' and '..' here is slightly different that for > canonicalize. Maybe just say that the raw joined path is canonicalizeed to avoid > duplicating the description. Done. http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:79: * `further.canonicalize()` starts with '../' or equals '..'. On 2012/06/18 07:29:35, Søren Gjesse wrote: > Is that sufficient? What about a [further] of 'x/../..'? 'x/../..'.canonicalize() is '..'. http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:88: * case is not implemented yet. On 2012/06/18 06:27:08, ajohnsen wrote: > A few comments here. > 1) Do you really mean "if no such path exists"? I thought this lib was purely > acting as a String manipulator? > 2) What do you mean by a case is not implemented? If something is not > implementable, we should write it here. And AFAIK, there is a few cases that'll > not work out? Done. http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:96: * the leading path separator if the path starts with a drive specification. On 2012/06/18 07:29:35, Søren Gjesse wrote: > Should the invariant here be that is the fromNative constructor was used that > toNativePath should return the exact same string? There are some corner cases, with mixed backward and forward slashes. But in general, yes. http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:98: String toNativePath(); On 2012/06/18 07:29:35, Søren Gjesse wrote: > The name toNativePath indicate that a Path not a String is returned. Could we say toNativeFilepath? toNativepath? toNativeFilesystemPath? http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:139: String get filename(); I think filename is a common noun, and not the same as file name. On 2012/06/18 07:29:35, Søren Gjesse wrote: > fileName (uppercase N)? http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path.dart#newc... runtime/bin/path.dart:159: String get extension(); On 2012/06/18 06:27:08, ajohnsen wrote: > With the classic case of .tar.gz, should we have a "fullExtension", or leave it > to the use to extract that? We could add that. or get extensions? multipleExtensions? http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path_impl.dart File runtime/bin/path_impl.dart (right): http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path_impl.dart... runtime/bin/path_impl.dart:22: var clean = source.replaceAll('\\', '/'); I think this would be more complex. I can't think of a name shorter or more expressive than '/'. On 2012/06/18 07:29:35, Søren Gjesse wrote: > Maybe create static final fields for '/', '\\', ':', '.' and '..'. > > static final String SEPARATOR = '/'; > ... http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path_impl.dart... runtime/bin/path_impl.dart:48: throw new Exception( On 2012/06/18 07:29:35, Søren Gjesse wrote: > We have UnsupportedOperationException and NotImplementedException - maybe use > one of these instead. Done. http://codereview.chromium.org/10417053/diff/18001/runtime/bin/path_impl.dart... runtime/bin/path_impl.dart:173: List result = path.split('/'); On 2012/06/18 07:29:35, Søren Gjesse wrote: > Maybe cache the result of splitting? I think this may be more trouble than it is worth. If the user needs this often, they could cache it. And splitting may be pretty fast. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
