Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19231002:
Port dart:io Path tests to package:path. (Closed)
|
Created:
7 years, 5 months ago by Bill Hesse Modified:
7 years, 5 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionPort dart:io Path tests to package:path.
BUG=
R=nweiz@google.com, rnystrom@google.com
Committed: https://code.google.com/p/dart/source/detail?r=25403
Patch Set 1 #Patch Set 2 : #
Total comments: 15
Patch Set 3 : Move tests to pkg/path/test directory. #Patch Set 4 : Merge with existing tests, remove redundant tests. #Patch Set 5 : Remove "import dart:io" from relative_test.dart #Patch Set 6 : Fix Windows failures #
Total comments: 33
Patch Set 7 : address comments #
Total comments: 8
Patch Set 8 : Address more comments. #Patch Set 9 : Add comment to relative(path, from: from) #
Messages
Total messages: 14 (0 generated)
This is not ready for LGTM - the test is not tested on Windows yet. But I want Bob to look at the proposed changes to path.dart - they are actually pretty small. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (left): https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#old... pkg/path/lib/path.dart:634: This is wrong if [from] is not null. It should work correctly with a null from as well. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#new... pkg/path/lib/path.dart:1118: // Normalize the Windows root if needed. oops.
Is your plan to convert the dart:io test to use pkg/path to catch regressions? Once that's done, do you intend to keep maintain a set of tests for this in test/standalone? I think it would be nice to remove the tests that are effectively duplicated between the two and then take the ones unique to the standalone tests and move them over to pkg/path/test. Thoughts? https://codereview.chromium.org/19231002/diff/2001/pkg/path/README.md File pkg/path/README.md (right): https://codereview.chromium.org/19231002/diff/2001/pkg/path/README.md#newcode82 pkg/path/README.md:82: If a relative path has no directories, then '.' is returned. You'll want a blank line after this. Markdown needs blank lines before and after code blocks. https://codereview.chromium.org/19231002/diff/2001/pkg/path/README.md#newcode85 pkg/path/README.md:85: I like this. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (left): https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#old... pkg/path/lib/path.dart:634: On 2013/07/15 16:58:48, Bill Hesse wrote: > This is wrong if [from] is not null. It should work correctly with a null from > as well. Eek, you're right. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#new... pkg/path/lib/path.dart:124: /// If a relative path has no directories, then '.' is returned. Blank line here. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#new... pkg/path/lib/path.dart:692: } Can you give me an example where this will come into play? I'd expect that fromParsed wouldn't have any ".." by the time you get here in most cases. We should add a test that hits this code path. If you can get to this case (maybe your builder's root is a relative path with a leading ".."?), I'd like to see if there's a "right" thing we can do instead of throwing an exception. People expect path manipulation operations to just work, so we try very hard not to throw exceptions. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#new... pkg/path/lib/path.dart:706: } Nice catch. Add a test for this? https://codereview.chromium.org/19231002/diff/2001/tests/standalone/io/path_t... File tests/standalone/io/path_test.dart (right): https://codereview.chromium.org/19231002/diff/2001/tests/standalone/io/path_t... tests/standalone/io/path_test.dart:8: import "package:path/path.dart" as Path; Lowercase "path" would be consistent with the other tests. https://codereview.chromium.org/19231002/diff/2001/tests/standalone/io/path_t... tests/standalone/io/path_test.dart:192: void relativeTest(Path.Builder P, String absolutePrefix) { "P" is a weird variable name. "builder" instead? That's what the other path tests do.
OK, all merged with the existing tests in pkg/path/test. Mainly new corner case tests have been added. https://codereview.chromium.org/19231002/diff/2001/pkg/path/README.md File pkg/path/README.md (right): https://codereview.chromium.org/19231002/diff/2001/pkg/path/README.md#newcode82 pkg/path/README.md:82: If a relative path has no directories, then '.' is returned. On 2013/07/15 17:49:29, Bob Nystrom wrote: > You'll want a blank line after this. Markdown needs blank lines before and after > code blocks. Done. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#new... pkg/path/lib/path.dart:124: /// If a relative path has no directories, then '.' is returned. On 2013/07/15 17:49:29, Bob Nystrom wrote: > Blank line here. Done. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#new... pkg/path/lib/path.dart:692: } On 2013/07/15 17:49:29, Bob Nystrom wrote: > Can you give me an example where this will come into play? I'd expect that > fromParsed wouldn't have any ".." by the time you get here in most cases. We > should add a test that hits this code path. > > If you can get to this case (maybe your builder's root is a relative path with a > leading ".."?), I'd like to see if there's a "right" thing we can do instead of > throwing an exception. People expect path manipulation operations to just work, > so we try very hard not to throw exceptions. My tests include this case. Any case with ".." in the from path that is not matched with one in the to path, when builder.root is '.'. I think the abstract case, where you want one relative path relative to another, without regard to where the paths are based, is important. I wish it could be done easier than making a new builder, but I can live with it. This truly is an exception - there is no good answer that makes sense for this request, and to rely on the result for anything would be an error. https://codereview.chromium.org/19231002/diff/2001/pkg/path/lib/path.dart#new... pkg/path/lib/path.dart:706: } On 2013/07/15 17:49:29, Bob Nystrom wrote: > Nice catch. Add a test for this? There is a test for this, in my tests. https://codereview.chromium.org/19231002/diff/2001/tests/standalone/io/path_t... File tests/standalone/io/path_test.dart (right): https://codereview.chromium.org/19231002/diff/2001/tests/standalone/io/path_t... tests/standalone/io/path_test.dart:8: import "package:path/path.dart" as Path; On 2013/07/15 17:49:29, Bob Nystrom wrote: > Lowercase "path" would be consistent with the other tests. I wanted to use "path" as a variable name, but I can change it. Done.
OK, tested on Linux, Mac, and Windows, with runtimes dart and dartium (content_shell) and d8, and compilers none and dart2js.
On 2013/07/16 14:37:57, Bill Hesse wrote: > OK, tested on Linux, Mac, and Windows, with runtimes dart and dartium > (content_shell) and d8, and compilers none and dart2js. PTAL. I'll be gone 'til Monday, so feel free to commit this.
Some comments, but I'm really excited about this change. There's no rush though, so we can just wait until you get back from vacation. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... File pkg/path/test/posix_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:170: expect(builder.join('a/', 'b/c/'), 'a/b/c/'); How about making a "preserves trailing separators" group that includes this and line 176? https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:173: expect(builder.join('a/b', 'c/../../../..'), 'a/b/c/../../../..'); This is a good test. Might be nice to have something similar for "/./". How about a group for "preserves '..' and '.'"? https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:174: expect(builder.join('', ''), ''); This is covered by line 158. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:175: expect(builder.join('a/b/c/', '/d'), '/d'); This is covered by line 140. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:176: expect(builder.join('a', 'b${builder.separator}'), 'a/b/'); You can just do "/" instead of ${builder.separator} here since this test is specifically for POSIX paths. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:233: expect(builder.normalize(r'\\'), r'\\'); Yay tests! https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:266: expect(builder.normalize('A:/../../..'), '../..'); Nice. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:461: void testGetters(String p, List components, String properties) { Do you think it's worth adding these? I find them a bit hard to read and I feel like the other tests for those functions probably cover this. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... File pkg/path/test/relative_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:14: Nit: just a single blank line between functions. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:15: void testRelativeTo() { Instead of a named function, how about just inlining this as a lambda in the call to test() above? https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:25: ''); Nit: this can go on the previous line. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:27: ''); This too. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:32: var m = absolutePrefix; How about "prefix"? That should still keep the test strings short but isn't meaningless like "m". https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:35: void test(result, p, from) { Nit: ditch the "void". https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:48: // Cases where the arguments are relative paths. Use a ":" here or a "." on the comment below. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:83: expect(() => builder.relative('a/b', from: '$m/a/b'), throws); I'm still not sure about these. My intuition is that, if the cwd is "/path/to/dir", then I would expect: builder.relative('.', from: '..') -> "dir" builder.relative('a/b', from: '../../d') -> "../to/dir/a/b" builder.relative('a/b', from: '$m/a/b') -> "../../path/to/dir/a/b" Is there something I'm missing? https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/url_test.dart File pkg/path/test/url_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/url_test.da... pkg/path/test/url_test.dart:7: Ditto comments on the POSIX tests here. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/windows_tes... File pkg/path/test/windows_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/windows_tes... pkg/path/test/windows_test.dart:4: And here.
https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... File pkg/path/test/posix_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:170: expect(builder.join('a/', 'b/c/'), 'a/b/c/'); On 2013/07/18 20:45:08, Bob Nystrom wrote: > How about making a "preserves trailing separators" group that includes this and > line 176? Done. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:173: expect(builder.join('a/b', 'c/../../../..'), 'a/b/c/../../../..'); On 2013/07/18 20:45:08, Bob Nystrom wrote: > This is a good test. Might be nice to have something similar for "/./". How > about a group for "preserves '..' and '.'"? Done. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:174: expect(builder.join('', ''), ''); On 2013/07/18 20:45:08, Bob Nystrom wrote: > This is covered by line 158. Done. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:175: expect(builder.join('a/b/c/', '/d'), '/d'); On 2013/07/18 20:45:08, Bob Nystrom wrote: > This is covered by line 140. Done. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:176: expect(builder.join('a', 'b${builder.separator}'), 'a/b/'); On 2013/07/18 20:45:08, Bob Nystrom wrote: > You can just do "/" instead of ${builder.separator} here since this test is > specifically for POSIX paths. We already have a test for literal / at the end. Since this is the idiom that is needed for platform-independent creation of a trailing slash, we should test it. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:461: void testGetters(String p, List components, String properties) { On 2013/07/18 20:45:08, Bob Nystrom wrote: > Do you think it's worth adding these? I find them a bit hard to read and I feel > like the other tests for those functions probably cover this. Removed. Some tests, like for '.', '..', and 'gule fisk.a b', added elsewhere. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... File pkg/path/test/relative_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:15: void testRelativeTo() { On 2013/07/18 20:45:08, Bob Nystrom wrote: > Instead of a named function, how about just inlining this as a lambda in the > call to test() above? Done. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:32: var m = absolutePrefix; On 2013/07/18 20:45:08, Bob Nystrom wrote: > How about "prefix"? That should still keep the test strings short but isn't > meaningless like "m". Done. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:48: // Cases where the arguments are relative paths. On 2013/07/18 20:45:08, Bob Nystrom wrote: > Use a ":" here or a "." on the comment below. Done. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:83: expect(() => builder.relative('a/b', from: '$m/a/b'), throws); On 2013/07/18 20:45:08, Bob Nystrom wrote: > I'm still not sure about these. My intuition is that, if the cwd is > "/path/to/dir", then I would expect: > > builder.relative('.', from: '..') -> "dir" > builder.relative('a/b', from: '../../d') -> "../to/dir/a/b" > builder.relative('a/b', from: '$m/a/b') -> "../../path/to/dir/a/b" > > Is there something I'm missing? This is for the case where cwd is '.'. When the user wants to work with paths relative to an unknown base, and get results that are correct no matter what the actual base is. The isRelative guard runs the tests only when there is no absolute cwd to resolve to.
https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... File pkg/path/test/posix_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:176: expect(builder.join('a', 'b${builder.separator}'), 'a/b/'); On 2013/07/22 11:35:22, Bill Hesse wrote: > On 2013/07/18 20:45:08, Bob Nystrom wrote: > > You can just do "/" instead of ${builder.separator} here since this test is > > specifically for POSIX paths. > > We already have a test for literal / at the end. > Since this is the idiom that is needed for platform-independent creation of a > trailing slash, we should test it. We already test what builder.separator returns, so I don't think this adds much value. Since builder is a explicitly a POSIX builder, this test is exactly identical to using an explicit "/". If, for some reason, separator were to ever return something different, the separator test would already fail. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... File pkg/path/test/relative_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:83: expect(() => builder.relative('a/b', from: '$m/a/b'), throws); On 2013/07/22 11:35:22, Bill Hesse wrote: > On 2013/07/18 20:45:08, Bob Nystrom wrote: > > I'm still not sure about these. My intuition is that, if the cwd is > > "/path/to/dir", then I would expect: > > > > builder.relative('.', from: '..') -> "dir" > > builder.relative('a/b', from: '../../d') -> "../to/dir/a/b" > > builder.relative('a/b', from: '$m/a/b') -> "../../path/to/dir/a/b" > > > > Is there something I'm missing? > > This is for the case where cwd is '.'. When the user wants to work with paths > relative to an unknown base, and get results that are correct no matter what the > actual base is. The isRelative guard runs the tests only when there is no > absolute cwd to resolve to. > Ah, that makes more sense now. If there are cases where Builder.relative can throw, we should document that clearly. Also, change "throws" to "throwsArgumentError" here.
https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... File pkg/path/test/posix_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:176: expect(builder.join('a', 'b${builder.separator}'), 'a/b/'); On 2013/07/22 11:35:22, Bill Hesse wrote: > On 2013/07/18 20:45:08, Bob Nystrom wrote: > > You can just do "/" instead of ${builder.separator} here since this test is > > specifically for POSIX paths. > > We already have a test for literal / at the end. > Since this is the idiom that is needed for platform-independent creation of a > trailing slash, we should test it. We already test what builder.separator returns, so I don't think this adds much value. Since builder is a explicitly a POSIX builder, this test is exactly identical to using an explicit "/". If, for some reason, separator were to ever return something different, the separator test would already fail. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... File pkg/path/test/relative_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:83: expect(() => builder.relative('a/b', from: '$m/a/b'), throws); On 2013/07/22 11:35:22, Bill Hesse wrote: > On 2013/07/18 20:45:08, Bob Nystrom wrote: > > I'm still not sure about these. My intuition is that, if the cwd is > > "/path/to/dir", then I would expect: > > > > builder.relative('.', from: '..') -> "dir" > > builder.relative('a/b', from: '../../d') -> "../to/dir/a/b" > > builder.relative('a/b', from: '$m/a/b') -> "../../path/to/dir/a/b" > > > > Is there something I'm missing? > > This is for the case where cwd is '.'. When the user wants to work with paths > relative to an unknown base, and get results that are correct no matter what the > actual base is. The isRelative guard runs the tests only when there is no > absolute cwd to resolve to. > Ah, that makes more sense now. If there are cases where Builder.relative can throw, we should document that clearly. Also, change "throws" to "throwsArgumentError" here.
https://codereview.chromium.org/19231002/diff/25001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/19231002/diff/25001/pkg/path/lib/path.dart#ne... pkg/path/lib/path.dart:689: // out of them. If a directory left in the from path is '..', it cannot Nit: single space after "." https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/posix_test.... File pkg/path/test/posix_test.dart (right): https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:186: test('Join does not modify internal ., .., or trailing separators', () { Nit: "join" should be lower-case https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/relative_te... File pkg/path/test/relative_test.dart (right): https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:15: 'c:'); It seems weird to me that the prefix here is "c:" when the actual root prefix on Windows is "c:\" (including the "\"). It's made more weird by the fact that "file:///" is a full prefix, and adding an additional "/" will result in a redundant extra "/". https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:29: test(result, p, from) { Calling this [test] is confusing given that [test] is a function defined by unittest and used elsewhere in this file. Maybe something like "expectRelative"? [p] is also a non-descriptive name. Maybe "pth"?
https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... File pkg/path/test/posix_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:176: expect(builder.join('a', 'b${builder.separator}'), 'a/b/'); On 2013/07/22 20:53:11, Bob Nystrom wrote: > On 2013/07/22 11:35:22, Bill Hesse wrote: > > On 2013/07/18 20:45:08, Bob Nystrom wrote: > > > You can just do "/" instead of ${builder.separator} here since this test is > > > specifically for POSIX paths. > > > > We already have a test for literal / at the end. > > Since this is the idiom that is needed for platform-independent creation of a > > trailing slash, we should test it. > > We already test what builder.separator returns, so I don't think this adds much > value. Since builder is a explicitly a POSIX builder, this test is exactly > identical to using an explicit "/". If, for some reason, separator were to ever > return something different, the separator test would already fail. Removed - I'm used to writing VM tests, where more tests, testing the same thing but written differently, are useful to find errors in the compiler and optimizer. If we assume the VM is working correctly, then this test is not needed. https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... File pkg/path/test/relative_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:83: expect(() => builder.relative('a/b', from: '$m/a/b'), throws); On 2013/07/22 20:53:11, Bob Nystrom wrote: > On 2013/07/22 11:35:22, Bill Hesse wrote: > > On 2013/07/18 20:45:08, Bob Nystrom wrote: > > > I'm still not sure about these. My intuition is that, if the cwd is > > > "/path/to/dir", then I would expect: > > > > > > builder.relative('.', from: '..') -> "dir" > > > builder.relative('a/b', from: '../../d') -> "../to/dir/a/b" > > > builder.relative('a/b', from: '$m/a/b') -> "../../path/to/dir/a/b" > > > > > > Is there something I'm missing? > > > > This is for the case where cwd is '.'. When the user wants to work with paths > > relative to an unknown base, and get results that are correct no matter what > the > > actual base is. The isRelative guard runs the tests only when there is no > > absolute cwd to resolve to. > > > > Ah, that makes more sense now. If there are cases where Builder.relative can > throw, we should document that clearly. > > Also, change "throws" to "throwsArgumentError" here. Done. Re the documentation, I just noticed, in looking at .relative's doc comments, that it says "If [from] is passed, [path] is made relative to that instead." This seems to imply that [root] is not used in that case, but the implementation does prepend both with [root] if they are relative. Should this be changed? The information about throwing ArgumentError can be added at the same time. https://codereview.chromium.org/19231002/diff/25001/pkg/path/lib/path.dart File pkg/path/lib/path.dart (right): https://codereview.chromium.org/19231002/diff/25001/pkg/path/lib/path.dart#ne... pkg/path/lib/path.dart:689: // out of them. If a directory left in the from path is '..', it cannot On 2013/07/22 21:00:33, nweiz wrote: > Nit: single space after "." Done. https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/posix_test.... File pkg/path/test/posix_test.dart (right): https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/posix_test.... pkg/path/test/posix_test.dart:186: test('Join does not modify internal ., .., or trailing separators', () { On 2013/07/22 21:00:33, nweiz wrote: > Nit: "join" should be lower-case Done. https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/relative_te... File pkg/path/test/relative_test.dart (right): https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:15: 'c:'); On 2013/07/22 21:00:33, nweiz wrote: > It seems weird to me that the prefix here is "c:" when the actual root prefix on > Windows is "c:\" (including the "\"). It's made more weird by the fact that > "file:///" is a full prefix, and adding an additional "/" will result in a > redundant extra "/". The prefix for the URL tests is 'http://myserver', not 'file:///'. 'file:///' is the builder's root. Changed all the prefixes to include the separator. https://codereview.chromium.org/19231002/diff/25001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:29: test(result, p, from) { On 2013/07/22 21:00:33, nweiz wrote: > Calling this [test] is confusing given that [test] is a function defined by > unittest and used elsewhere in this file. Maybe something like "expectRelative"? > > [p] is also a non-descriptive name. Maybe "pth"? Done.
lgtm https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... File pkg/path/test/relative_test.dart (right): https://codereview.chromium.org/19231002/diff/17001/pkg/path/test/relative_te... pkg/path/test/relative_test.dart:83: expect(() => builder.relative('a/b', from: '$m/a/b'), throws); On 2013/07/23 16:07:00, Bill Hesse wrote: > On 2013/07/22 20:53:11, Bob Nystrom wrote: > > On 2013/07/22 11:35:22, Bill Hesse wrote: > > > On 2013/07/18 20:45:08, Bob Nystrom wrote: > > > > I'm still not sure about these. My intuition is that, if the cwd is > > > > "/path/to/dir", then I would expect: > > > > > > > > builder.relative('.', from: '..') -> "dir" > > > > builder.relative('a/b', from: '../../d') -> "../to/dir/a/b" > > > > builder.relative('a/b', from: '$m/a/b') -> "../../path/to/dir/a/b" > > > > > > > > Is there something I'm missing? > > > > > > This is for the case where cwd is '.'. When the user wants to work with > paths > > > relative to an unknown base, and get results that are correct no matter what > > the > > > actual base is. The isRelative guard runs the tests only when there is no > > > absolute cwd to resolve to. > > > > > > > Ah, that makes more sense now. If there are cases where Builder.relative can > > throw, we should document that clearly. > > > > Also, change "throws" to "throwsArgumentError" here. > > Done. > > Re the documentation, I just noticed, in looking at .relative's doc > comments, that it says "If [from] is passed, [path] is made relative to that > instead." This seems to imply that [root] is not used in that case, but the > implementation does prepend both with [root] if they are relative. Should this > be changed? The information about throwing ArgumentError can be added at the > same time. I think it's implicit that [path] may be relative to [from] using information known about [root], but it wouldn't hurt to make that more explicit in the documentation.
LGTM and +1 to Nathan's comments. Thanks for this!
Message was sent while issue was closed.
Committed patchset #9 manually as r25403 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
