| === mup_ is now known as mup | ||
| === StoneTable is now known as aisrael | ||
| === bodie__ is now known as bodie_ | ||
| === bodie_ is now known as Guest39866 | ||
| === jcsackett_ is now known as jcsackett | ||
| === meetingology` is now known as meetingology | ||
| === ev_ is now known as ev | ||
| === Guest39866 is now known as bodie_ | ||
| bogdanteleaga | hello, could I get another review for https://github.com/juju/testing/pull/29 ? | 15:33 |
|---|---|---|
| bodie_ | bogdanteleaga, the PathChecker is only for checking that the paths are the same, right? it shouldn't matter whether there is a file there | 17:15 |
| bodie_ | I guess the problem is that "C:/Program Files" and "C:/Programming" have a name collision and could be C:/PROGRA~1 and C:/PROGRA~2 | 17:16 |
| bodie_ | but can those really be considered the same *path* | 17:16 |
| bodie_ | it's possible that there is a problem with my concept of "path" | 17:17 |
| bogdanteleaga | Umm, it uses os.samefile to check | 17:18 |
| bogdanteleaga | So it would actually work | 17:18 |
| bogdanteleaga | It's kind of a workaround for symlinks | 17:18 |
| bogdanteleaga | Because they don't work on windows | 17:18 |
| bogdanteleaga | And yeah it doesn't matter if there's a file there | 17:18 |
| bogdanteleaga | It's multipurpose | 17:18 |
| bogdanteleaga | If the path doesn't match it checks if it points to the same file | 17:19 |
| bodie_ | no, I know it would work in that case, the question is more whether the files should have to exist for the path to check correctly | 17:20 |
| bodie_ | i.e., maybe C:/PROGRA~1 matches both C:/Program Files and C:/Programming | 17:21 |
| bogdanteleaga | It wouldn't | 17:22 |
| bodie_ | well it's not a one-to-one mapping, no, rather a many-to-many, so the concept of comparing those paths is intrinsically problematic unless you compare the underlying files | 17:22 |
| bodie_ | my thought is only that perhaps if you're comparing paths, you don't want the actual files to have to exist | 17:23 |
| bodie_ | but, it's just a thought :) | 17:23 |
| bogdanteleaga | they don't have to | 17:24 |
| bogdanteleaga | if they don't exist it exits | 17:24 |
| bogdanteleaga | if you follow the logic it first looks at the strings themselves | 17:25 |
| bogdanteleaga | and if they don't match then it looks if both of them point to actual files | 17:25 |
| bodie_ | I'm specifically looking at your comment on line 205 | 17:25 |
| bogdanteleaga | and only after that if checks whether the files are actually the same file | 17:25 |
| bodie_ | thus, the files don't have to exist _unless_ x | 17:26 |
| bodie_ | but, I'm saying C:/Programming could a match for C:/PROGRA~1 even if neither file exists | 17:26 |
| bodie_ | or PROGRA~2 | 17:27 |
| bodie_ | there's really no way of knowing | 17:27 |
| bodie_ | since it's many-to-many name mapping | 17:27 |
| bodie_ | I just don't know if the user will expect the files to have to exist in that case | 17:27 |
| bogdanteleaga | they would not match because if they don't exist it would throw that specific error | 17:27 |
| bogdanteleaga | the only case in which you would expect 2 paths to be the same if they contain different strings is the case in which they point to the same file | 17:28 |
| bogdanteleaga | I don't understand exactly what you're getting at here | 17:28 |
| bogdanteleaga | think of specific use cases | 17:28 |
| bodie_ | well what you're testing is whether the files match in those cases, not whether the paths match | 17:29 |
| bodie_ | and if the files don't exist, and the user is trying to check something about paths, maybe that could be problematic | 17:29 |
| bodie_ | however, I'm not truly certain about your intent, so I thought I would just mention it here instead of holding back your review | 17:29 |
| bodie_ | as I said -- just a thought :) | 17:30 |
| bogdanteleaga | yeah I got that it's just a thought but I wanted to clarify it for you | 17:30 |
| bogdanteleaga | the issue is that if the paths are not the same string-wise the only other possible case is a symlink or hardlink | 17:31 |
| bodie_ | hmm, okay | 17:31 |
| bogdanteleaga | it's probably only going to get used inside windows tests, but it's also *nix compatible | 17:32 |
| bogdanteleaga | thanks for the review, i'll fix that in a bit | 17:44 |
| bogdanteleaga | actually I had a test for that already | 17:49 |
| bogdanteleaga | commented on github | 17:49 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!