Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11553005:
Move path-manipulation code from io.dart into path.dart. (Closed)
|
Created:
8 years ago by nweiz Modified:
8 years ago Reviewers:
Bob Nystrom CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMove path-manipulation code from io.dart into path.dart.
Committed: https://code.google.com/p/dart/source/detail?r=16063
Patch Set 1 #
Total comments: 58
Patch Set 2 : Code review changes #Patch Set 3 : Bug fix #Patch Set 4 : Code review changes + relative fix #
Messages
Total messages: 6 (0 generated)
Keeping track of separators and treating "a/" as different than "a" caused me headaches when implementing split() and roughly doubled the size of the dirname() implementation. If we didn't test so extensively, it would definitely have led to some subtle bugs going undetected.
https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart File utils/pub/path.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode62 utils/pub/path.dart:62: /// Returns the root of [path], if it's absolute, or `null` if it's relative. Generally, this library returns empty strings instead of null. That way you can rely on getting a valid string out. How about doing that here too? https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode71 utils/pub/path.dart:71: String rootOf(String path) => _builder.rootOf(path); "rootOf" -> "root", I think. I like English-like APIs but that feels a bit too much to me. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode116 utils/pub/path.dart:116: List<String> split(String path) => _builder.split(path); Heh, my preflight patch has an implementation of this too. For mine, I put the prefix in the first part: ['/path', 'foo', 'bar']. Does that work for you? https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode131 utils/pub/path.dart:131: /// If the [to] argument is passed, [path] is made relative to that instead. I don't like reinterpreting one argument based on whether or not another one was passed. Can you do "from:" instead of "to:" here? https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode185 utils/pub/path.dart:185: /// builder.dirname('path/to'); // -> 'to' Show an example of what happens if there's a trailing separator too. :) https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode186 utils/pub/path.dart:186: String dirname(String path) { Nit, but can you move this after basenameWithoutExtension. The methods here are alphabetical (mostly). https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode188 utils/pub/path.dart:188: if (parsed.parts.isEmpty) return '.'; You'll need to handle absolute paths here. If there's a root, return that instead of '.'. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode189 utils/pub/path.dart:189: if (parsed.separators.last == '') { !parsed.hasTrailingSeparator https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode190 utils/pub/path.dart:190: if (parsed.parts.length == 1) return '.'; I think you should be able to skip this check. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode194 utils/pub/path.dart:194: parsed.separators[parsed.separators.length - 1] = ''; What's this for? https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode271 utils/pub/path.dart:271: if (parts[i] == null || parts[i - 1] != null) continue; I'd find this more intuitive if you flipped the logic: parts[i] != null && parts[i - 1] == null https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode303 utils/pub/path.dart:303: /// Splits [path] into its components using the current platform's [separator]. Long line. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... File utils/tests/pub/path/path_posix_test.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:39: expect(builder.rootOf('a/b'), null); I think these should return empty strings. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:52: expect(builder.dirname(r'a\b/c'), r'a\b'); Can you add tests for: / -> / a/b/ -> a/b/ (subject to discussion, i suppose :) ) a/b\c -> a a// -> a (?) Also, matching tests on windows. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:133: expect(builder.join('a', null), equals('a')); Do we want to allow join() with no arguments? It could return ".". https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:133: expect(builder.join('a', null), equals('a')); Everything else doesn't use explicit equals() here. I don't think it adds any value and it adds more boilerplate to the tests. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:151: expect(builder.split('foo/\\/baz'), equals(['foo', '\\', 'baz'])); Also tests for: . -> . '' -> [] foo/ -> [foo] // -> [/] ? https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:290: expect(builder.relative('/foo/bar/baz', to: '/foo/bar'), equals('baz')); This is confusing. Does "to" mean "from" here? Isn't the path from "/foo/bar/baz" to "/foo/bar" ".."? https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... File utils/tests/pub/path/path_windows_test.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... utils/tests/pub/path/path_windows_test.dart:42: expect(builder.rootOf(r'C:\a\c'), 'C:\\'); Can you use raw strings for these? Here and below. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... utils/tests/pub/path/path_windows_test.dart:174: expect(builder.split(r'C:\foo\bar\baz'), equals(['C:\\', 'foo', 'bar', 'baz'])); Long line. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... utils/tests/pub/path/path_windows_test.dart:319: expect(builder.relative(r'C:\foo\bar\baz', to: r'C:\foo\bar'), equals('baz')); Long line.
https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart File utils/pub/path.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode62 utils/pub/path.dart:62: /// Returns the root of [path], if it's absolute, or `null` if it's relative. On 2012/12/12 00:01:20, Bob Nystrom wrote: > Generally, this library returns empty strings instead of null. That way you can > rely on getting a valid string out. How about doing that here too? I based it on the ParsedPath API, which also returns null here. I think null makes sense when you're indicating the absence of the property you're querying for. Also, the empty string won't work any more correctly if you try to treat it as the root in e.g. join(). https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode71 utils/pub/path.dart:71: String rootOf(String path) => _builder.rootOf(path); On 2012/12/12 00:01:20, Bob Nystrom wrote: > "rootOf" -> "root", I think. I like English-like APIs but that feels a bit too > much to me. I had it as "root" originally, but that causes a name conflict with Builder.root. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode116 utils/pub/path.dart:116: List<String> split(String path) => _builder.split(path); On 2012/12/12 00:01:20, Bob Nystrom wrote: > Heh, my preflight patch has an implementation of this too. For mine, I put the > prefix in the first part: ['/path', 'foo', 'bar']. Does that work for you? I like this version better, since it lists all directories in the original path whereas ['/path', 'foo', 'bar'] leaves off '/'. That makes it harder to e.g. check if there's any parent directory with a given name. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode131 utils/pub/path.dart:131: /// If the [to] argument is passed, [path] is made relative to that instead. On 2012/12/12 00:01:20, Bob Nystrom wrote: > I don't like reinterpreting one argument based on whether or not another one was > passed. Can you do "from:" instead of "to:" here? The idea is that it returns "path" relative to "to". "to" is effectively short for "relativeTo". I'm going to change it to "base" to sidestep any attempts to think of it as an English phrase, since that can evidently be interpreted in two conflicting ways. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode185 utils/pub/path.dart:185: /// builder.dirname('path/to'); // -> 'to' On 2012/12/12 00:01:20, Bob Nystrom wrote: > Show an example of what happens if there's a trailing separator too. :) I left that out intentionally because I want to change that behavior in the near future. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode186 utils/pub/path.dart:186: String dirname(String path) { On 2012/12/12 00:01:20, Bob Nystrom wrote: > Nit, but can you move this after basenameWithoutExtension. The methods here are > alphabetical (mostly). Done. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode188 utils/pub/path.dart:188: if (parsed.parts.isEmpty) return '.'; On 2012/12/12 00:01:20, Bob Nystrom wrote: > You'll need to handle absolute paths here. If there's a root, return that > instead of '.'. Done. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode189 utils/pub/path.dart:189: if (parsed.separators.last == '') { On 2012/12/12 00:01:20, Bob Nystrom wrote: > !parsed.hasTrailingSeparator Done. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode190 utils/pub/path.dart:190: if (parsed.parts.length == 1) return '.'; On 2012/12/12 00:01:20, Bob Nystrom wrote: > I think you should be able to skip this check. It dirname("a") should return ".", not "". https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode194 utils/pub/path.dart:194: parsed.separators[parsed.separators.length - 1] = ''; On 2012/12/12 00:01:20, Bob Nystrom wrote: > What's this for? For paths with trailing separators, this makes dirname("a/") return "a". I don't like it, but it is consistent with the behavior of basename(). For paths without trailing separators, this makes dirname("a/b") return "a" rather than "a/". https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode271 utils/pub/path.dart:271: if (parts[i] == null || parts[i - 1] != null) continue; On 2012/12/12 00:01:20, Bob Nystrom wrote: > I'd find this more intuitive if you flipped the logic: > > parts[i] != null && parts[i - 1] == null Done. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode303 utils/pub/path.dart:303: /// Splits [path] into its components using the current platform's [separator]. On 2012/12/12 00:01:20, Bob Nystrom wrote: > Long line. Done. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... File utils/tests/pub/path/path_posix_test.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:39: expect(builder.rootOf('a/b'), null); On 2012/12/12 00:01:20, Bob Nystrom wrote: > I think these should return empty strings. See other reply. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:52: expect(builder.dirname(r'a\b/c'), r'a\b'); On 2012/12/12 00:01:20, Bob Nystrom wrote: > Can you add tests for: > > / -> / Done. > a/b/ -> a/b/ (subject to discussion, i suppose :) ) The current semantics have this returning "a/b", although it should return "a". > a/b\c -> a Done. > a// -> a (?) Current semantics have this returning "a/", although it should return ".". https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:133: expect(builder.join('a', null), equals('a')); On 2012/12/12 00:01:20, Bob Nystrom wrote: > Do we want to allow join() with no arguments? It could return ".". I don't think so. Given that we don't have a way to pass varargs, there's no real use for it. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:133: expect(builder.join('a', null), equals('a')); On 2012/12/12 00:01:20, Bob Nystrom wrote: > Everything else doesn't use explicit equals() here. I don't think it adds any > value and it adds more boilerplate to the tests. Every other test in Dart (not to mention Pub) uses it; I think we should follow the project style. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:151: expect(builder.split('foo/\\/baz'), equals(['foo', '\\', 'baz'])); On 2012/12/12 00:01:20, Bob Nystrom wrote: > Also tests for: > > . -> . > '' -> [] > foo/ -> [foo] > // -> [/] ? Done. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:290: expect(builder.relative('/foo/bar/baz', to: '/foo/bar'), equals('baz')); On 2012/12/12 00:01:20, Bob Nystrom wrote: > This is confusing. Does "to" mean "from" here? Isn't the path from > "/foo/bar/baz" to "/foo/bar" ".."? Changed "to" to "base". https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... File utils/tests/pub/path/path_windows_test.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... utils/tests/pub/path/path_windows_test.dart:42: expect(builder.rootOf(r'C:\a\c'), 'C:\\'); On 2012/12/12 00:01:20, Bob Nystrom wrote: > Can you use raw strings for these? Here and below. It screws with my editor to have a string ending with "\", even if it's a raw string :-/. I can make them raw strings if you really want, but it'll make my life tougher for relatively little gain. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... utils/tests/pub/path/path_windows_test.dart:174: expect(builder.split(r'C:\foo\bar\baz'), equals(['C:\\', 'foo', 'bar', 'baz'])); On 2012/12/12 00:01:20, Bob Nystrom wrote: > Long line. Done. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... utils/tests/pub/path/path_windows_test.dart:319: expect(builder.relative(r'C:\foo\bar\baz', to: r'C:\foo\bar'), equals('baz')); On 2012/12/12 00:01:20, Bob Nystrom wrote: > Long line. Done.
https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart File utils/pub/path.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode62 utils/pub/path.dart:62: /// Returns the root of [path], if it's absolute, or `null` if it's relative. On 2012/12/12 00:50:37, nweiz wrote: > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > Generally, this library returns empty strings instead of null. That way you > can > > rely on getting a valid string out. How about doing that here too? > > I based it on the ParsedPath API, which also returns null here. That's part of the reason ParsedPath is still private. > I think null makes sense when you're indicating the absence of the property you're querying > for. Also, the empty string won't work any more correctly if you try to treat it > as the root in e.g. join(). But it won't work *less* correctly there, will it? I really think we should avoid returning null here. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode71 utils/pub/path.dart:71: String rootOf(String path) => _builder.rootOf(path); On 2012/12/12 00:50:37, nweiz wrote: > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > "rootOf" -> "root", I think. I like English-like APIs but that feels a bit too > > much to me. > > I had it as "root" originally, but that causes a name conflict with > Builder.root. maybe rootPrefix? https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode116 utils/pub/path.dart:116: List<String> split(String path) => _builder.split(path); On 2012/12/12 00:50:37, nweiz wrote: > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > Heh, my preflight patch has an implementation of this too. For mine, I put the > > prefix in the first part: ['/path', 'foo', 'bar']. Does that work for you? > > I like this version better, since it lists all directories in the original path > whereas ['/path', 'foo', 'bar'] leaves off '/'. That makes it harder to e.g. > check if there's any parent directory with a given name. Ah, good call. Works for me. https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode131 utils/pub/path.dart:131: /// If the [to] argument is passed, [path] is made relative to that instead. On 2012/12/12 00:50:37, nweiz wrote: > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > I don't like reinterpreting one argument based on whether or not another one > was > > passed. Can you do "from:" instead of "to:" here? > > The idea is that it returns "path" relative to "to". "to" is effectively short > for "relativeTo". > > I'm going to change it to "base" to sidestep any attempts to think of it as an > English phrase, since that can evidently be interpreted in two conflicting ways. Before I accidentally lost the implementation, I was using from: here, and I liked it: path.relative('foo/bar', from: 'foo') -> 'bar' https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode190 utils/pub/path.dart:190: if (parsed.parts.length == 1) return '.'; On 2012/12/12 00:50:37, nweiz wrote: > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > I think you should be able to skip this check. > > It dirname("a") should return ".", not "". Right, but I believe toString() handles that, doesn't it? https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode194 utils/pub/path.dart:194: parsed.separators[parsed.separators.length - 1] = ''; On 2012/12/12 00:50:37, nweiz wrote: > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > What's this for? > > For paths with trailing separators, this makes dirname("a/") return "a". I don't > like it, but it is consistent with the behavior of basename(). Yeah, consistency is what matters most to me here. > > For paths without trailing separators, this makes dirname("a/b") return "a" > rather than "a/". OK. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... File utils/tests/pub/path/path_posix_test.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:133: expect(builder.join('a', null), equals('a')); On 2012/12/12 00:50:37, nweiz wrote: > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > Everything else doesn't use explicit equals() here. I don't think it adds any > > value and it adds more boilerplate to the tests. > > Every other test in Dart (not to mention Pub) uses it; I think we should follow > the project style. This will get pulled out of pub before too long, so consistency there doesn't matter much to me. What's so bad about this? It's a well-defined part of the unittest API? https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... File utils/tests/pub/path/path_windows_test.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win... utils/tests/pub/path/path_windows_test.dart:42: expect(builder.rootOf(r'C:\a\c'), 'C:\\'); On 2012/12/12 00:50:37, nweiz wrote: > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > Can you use raw strings for these? Here and below. > > It screws with my editor to have a string ending with "\", even if it's a raw > string :-/. I can make them raw strings if you really want, but it'll make my > life tougher for relatively little gain. It screws up my editor too (I need to fix the sublime bundle) but I do really want raw strings here. I tripped over reading these a bunch of times.
This latest batch of changes also fixes Builder.relative so that it doesn't
access the current working directory. This makes it safe to use with reference
to foreign file systems.
This does cause the following code to throw an ArgumentError:
new Builder('relative').relative('.', from: '/absolute')
This is necessary since there is no known path from "/absolute" to "relative".
Fortunately this will rarely come up, since Builders will very rarely be given
relative roots.
https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart
File utils/pub/path.dart (right):
https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode62
utils/pub/path.dart:62: /// Returns the root of [path], if it's absolute, or
`null` if it's relative.
On 2012/12/12 01:02:13, Bob Nystrom wrote:
> On 2012/12/12 00:50:37, nweiz wrote:
> > On 2012/12/12 00:01:20, Bob Nystrom wrote:
> > > Generally, this library returns empty strings instead of null. That way
you
> > can
> > > rely on getting a valid string out. How about doing that here too?
> >
> > I based it on the ParsedPath API, which also returns null here.
>
> That's part of the reason ParsedPath is still private.
>
> > I think null makes sense when you're indicating the absence of the property
> you're querying
> > for. Also, the empty string won't work any more correctly if you try to
treat
> it
> > as the root in e.g. join().
>
> But it won't work *less* correctly there, will it? I really think we should
> avoid returning null here.
Done.
https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode71
utils/pub/path.dart:71: String rootOf(String path) => _builder.rootOf(path);
On 2012/12/12 01:02:13, Bob Nystrom wrote:
> On 2012/12/12 00:50:37, nweiz wrote:
> > On 2012/12/12 00:01:20, Bob Nystrom wrote:
> > > "rootOf" -> "root", I think. I like English-like APIs but that feels a bit
> too
> > > much to me.
> >
> > I had it as "root" originally, but that causes a name conflict with
> > Builder.root.
>
> maybe rootPrefix?
Done.
https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode131
utils/pub/path.dart:131: /// If the [to] argument is passed, [path] is made
relative to that instead.
On 2012/12/12 01:02:13, Bob Nystrom wrote:
> On 2012/12/12 00:50:37, nweiz wrote:
> > On 2012/12/12 00:01:20, Bob Nystrom wrote:
> > > I don't like reinterpreting one argument based on whether or not another
one
> > was
> > > passed. Can you do "from:" instead of "to:" here?
> >
> > The idea is that it returns "path" relative to "to". "to" is effectively
short
> > for "relativeTo".
> >
> > I'm going to change it to "base" to sidestep any attempts to think of it as
an
> > English phrase, since that can evidently be interpreted in two conflicting
> ways.
>
> Before I accidentally lost the implementation, I was using from: here, and I
> liked it:
>
> path.relative('foo/bar', from: 'foo') -> 'bar'
That reads as "'foo/bar' relative from 'foo'" to me, which sounds awkward, but
if you like it I'll make the switch.
https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode190
utils/pub/path.dart:190: if (parsed.parts.length == 1) return '.';
On 2012/12/12 01:02:13, Bob Nystrom wrote:
> On 2012/12/12 00:50:37, nweiz wrote:
> > On 2012/12/12 00:01:20, Bob Nystrom wrote:
> > > I think you should be able to skip this check.
> >
> > It dirname("a") should return ".", not "".
>
> Right, but I believe toString() handles that, doesn't it?
No, it would just return ''.
https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos...
File utils/tests/pub/path/path_posix_test.dart (right):
https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos...
utils/tests/pub/path/path_posix_test.dart:133: expect(builder.join('a', null),
equals('a'));
On 2012/12/12 01:02:13, Bob Nystrom wrote:
> On 2012/12/12 00:50:37, nweiz wrote:
> > On 2012/12/12 00:01:20, Bob Nystrom wrote:
> > > Everything else doesn't use explicit equals() here. I don't think it adds
> any
> > > value and it adds more boilerplate to the tests.
> >
> > Every other test in Dart (not to mention Pub) uses it; I think we should
> follow
> > the project style.
>
> This will get pulled out of pub before too long, so consistency there doesn't
> matter much to me. What's so bad about this? It's a well-defined part of the
> unittest API?
In everything else we stick with the Dart project style. We even poke fun at
dart2js for not following Dart style. It's certainly the Dart style to use
equals() explicitly, and I feel like we should follow the Dart style even when
we don't like it if we expect others to do the same.
https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win...
File utils/tests/pub/path/path_windows_test.dart (right):
https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:42:
expect(builder.rootOf(r'C:\a\c'), 'C:\\');
On 2012/12/12 01:02:13, Bob Nystrom wrote:
> On 2012/12/12 00:50:37, nweiz wrote:
> > On 2012/12/12 00:01:20, Bob Nystrom wrote:
> > > Can you use raw strings for these? Here and below.
> >
> > It screws with my editor to have a string ending with "\", even if it's a
raw
> > string :-/. I can make them raw strings if you really want, but it'll make
my
> > life tougher for relatively little gain.
>
> It screws up my editor too (I need to fix the sublime bundle) but I do really
> want raw strings here. I tripped over reading these a bunch of times.
Done.
LGTM! https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart File utils/pub/path.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/pub/path.dart#newcode131 utils/pub/path.dart:131: /// If the [to] argument is passed, [path] is made relative to that instead. On 2012/12/12 02:45:57, nweiz wrote: > On 2012/12/12 01:02:13, Bob Nystrom wrote: > > On 2012/12/12 00:50:37, nweiz wrote: > > > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > > > I don't like reinterpreting one argument based on whether or not another > one > > > was > > > > passed. Can you do "from:" instead of "to:" here? > > > > > > The idea is that it returns "path" relative to "to". "to" is effectively > short > > > for "relativeTo". > > > > > > I'm going to change it to "base" to sidestep any attempts to think of it as > an > > > English phrase, since that can evidently be interpreted in two conflicting > > ways. > > > > Before I accidentally lost the implementation, I was using from: here, and I > > liked it: > > > > path.relative('foo/bar', from: 'foo') -> 'bar' > > That reads as "'foo/bar' relative from 'foo'" to me, which sounds awkward, but > if you like it I'll make the switch. Yeah, I like it a bit more. I read it as "get a path relative to 'foo/bar' from 'foo'" which has the nouns in about the right order. https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... File utils/tests/pub/path/path_posix_test.dart (right): https://codereview.chromium.org/11553005/diff/1/utils/tests/pub/path/path_pos... utils/tests/pub/path/path_posix_test.dart:133: expect(builder.join('a', null), equals('a')); On 2012/12/12 02:45:57, nweiz wrote: > On 2012/12/12 01:02:13, Bob Nystrom wrote: > > On 2012/12/12 00:50:37, nweiz wrote: > > > On 2012/12/12 00:01:20, Bob Nystrom wrote: > > > > Everything else doesn't use explicit equals() here. I don't think it adds > > any > > > > value and it adds more boilerplate to the tests. > > > > > > Every other test in Dart (not to mention Pub) uses it; I think we should > > follow > > > the project style. > > > > This will get pulled out of pub before too long, so consistency there doesn't > > matter much to me. What's so bad about this? It's a well-defined part of the > > unittest API? > > In everything else we stick with the Dart project style. We even poke fun at > dart2js for not following Dart style. It's certainly the Dart style to use > equals() explicitly, and I feel like we should follow the Dart style even when > we don't like it if we expect others to do the same. Sure, if it matters that much to you. In that case, I wonder if it's worth making little one line convenience functions: expectJoin(arg, expected) { expect(builder.join(arg), equals(expected)); } I'm just trying to minimize the clutter around the args and expected result. Either way though, I'm fine with changing it. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
