We need a module comparable to Python os.path.

The API. Implementation for Unix should be sufficient, implementation for Windows is incomplete. Also missing a number of well-known directories.

Comment on attachment 581686 [details] [diff] [review] v1. The API i'm not the right person to review; I'd ask one of the module owners of toolkit: https://wiki.mozilla.org/Modules/Toolkit Dave

Comment on attachment 634416 [details] [diff] [review] Unix back-end Taras, I have found myself blocked a few times because we have no portable way to represent paths in OS.File, so I have put together a prototype implementation. Could you take a look? This implements only the data structure. To make it useful, I intend to add a function |Path.to| that The idea is that once we have this OS.Path to add a function |Path.to| that will hook to the DirectoryProvider through some native code.

Comment on attachment 634416 [details] [diff] [review] Unix back-end I prefer the python way over OO here. See http://docs.python.org/library/os.path.html

Can't sleep, so here is a revised version. Taras, can you tell me what you think of this one? If it works for you, I will start working on the dreaded Windows implementation.

Comment on attachment 634624 [details] [diff] [review] Unix back-end i could if bugzilla dns wasn't busted :(

Ah well, still can't sleep. Here is a prototype of the Windows back-end. I still believe that we should introduce a data structure for paths. In particular, this would let us ensure that we only |join| and |split| normalized paths, without performance penalty.

We can even combine both. Leave these back-ends with the rest of the back-ends for users who prefer working with strings, and add a thin front-end to wrap them in safer/more abstract OO code.

Of course, I forgot to mention, but the main reason to add a little abstraction on top of the API is to ensure that users concatenate paths with |join| and not manually, which is quite brittle.

Comment on attachment 634694 [details] [diff] [review] Windows test suite Really? OS.Unix.Path in the windows testsuite? This works?

Comment on attachment 634696 [details] [diff] [review] Windows back-end exports.OS.Unix.Path <-- do not use Unix on a windows platform. If for whatever OS.Path sure, but not OS.lie.Path. + basename: function basename(path) { + return path.slice(Math.max(path.lastIndexOf("\\"), path.lastIndexOf(":")) + 1); + }, This and other path handling functions need to explicitly block paths(ie UNC ones) that start with '\\'(make a utility). Make a utility function that throws an exception if a UNC path is detected. Otherwise you get weird string exceptions for paths without ':' in them. Should also check errors when looking for "\\" position in path strings and throw proper exceptions. + let noDrive = (options && options.winNoDrive); what the heck is .winNoDrive? no docs for it. Need to document what you are doing in join with regexps, general strategy, etc. I generally don't require comments in the body if the code is clear, it's not in this case.

Comment on attachment 634624 [details] [diff] [review] Unix back-end + return path.slice(path.lastIndexOf("/") + 1); as in other patch, please check errors from string functions and throw appropriate exceptions.

(In reply to Taras Glek (:taras) from comment #15) > Comment on attachment 634694 [details] [diff] [review] > Windows test suite > > Really? OS.Unix.Path in the windows testsuite? This works? Sorry, that's an artifact of testing the Windows version on a Unix computer. I'll fix this.

(In reply to Taras Glek (:taras) from comment #16) > Comment on attachment 634696 [details] [diff] [review] > Windows back-end > > > exports.OS.Unix.Path <-- do not use Unix on a windows platform. If for > whatever OS.Path sure, but not OS.lie.Path. Well, it is not a lie, it is an error, but yes, it is still my fault for uploading at 3am. > > > + basename: function basename(path) { > + return path.slice(Math.max(path.lastIndexOf("\\"), > path.lastIndexOf(":")) + 1); > + }, > > This and other path handling functions need to explicitly block paths(ie UNC > ones) that start with '\\'(make a utility). Make a utility function that > throws an exception if a UNC path is detected. > > Otherwise you get weird string exceptions for paths without ':' in them. > Should also check errors when looking for "\\" position in path strings and > throw proper exceptions. Will do. I still think that this check would be better served as part of a data structure, though. Do you mind if I open a bug for that? > > + let noDrive = (options && options.winNoDrive); > > what the heck is .winNoDrive? no docs for it. I will document this. > Need to document what you are doing in join with regexps, general strategy, > etc. I generally don't require comments in the body if the code is clear, > it's not in this case. Ok.

(In reply to Taras Glek (:taras) from comment #17) > Comment on attachment 634624 [details] [diff] [review] > Unix back-end > > + return path.slice(path.lastIndexOf("/") + 1); > > as in other patch, please check errors from string functions and throw > appropriate exceptions. The only way these functions can fail is if |path| is |null|. Is this what you want me to check?

Updating the Windows back-end: more comments, more doc, and checks that we are not

(In reply to David Rajchenbach Teller [:Yoric] from comment #20) > (In reply to Taras Glek (:taras) from comment #17) > > Comment on attachment 634624 [details] [diff] [review] > > Unix back-end > > > > + return path.slice(path.lastIndexOf("/") + 1); > > > > as in other patch, please check errors from string functions and throw > > appropriate exceptions. > > The only way these functions can fail is if |path| is |null|. Is this what > you want me to check? good point, this is a false alarm. I just want to ensure we don't throw string exceptions out of os.file.

Comment on attachment 635325 [details] [diff] [review] Windows back-end This is getting close to landing, a few minor cleanups. + return (!noDrive && this.winGetDrive(path)) || "."; This is too clever. Please use an if statement here. + // Ignore any occurrence of "\\: immediately before that one typo + start = (this.winGetDrive(path) || "").length; too clever. if please + for each(let i in arguments) { s/i/segment, component, whatever you like/ i implies integer-like thing + let result = paths.join("\\"); + if (absolute) { + result = "\\" + result; + } I'm not a fan of prepending strings. Instead of a prepend, do an append..ie add drive letter first, result after. Same with if(root) below. + winGetDrive: function winHasDrive(path) { <-- that's bad Name indicates boolean, return is a string/null. call it winGetDrive similar thing with winIsAbsolute -> winEnsureAbsolute

(In reply to Taras Glek (:taras) from comment #26) > Comment on attachment 635325 [details] [diff] [review] > Windows back-end > > This is getting close to landing, a few minor cleanups. > > + return (!noDrive && this.winGetDrive(path)) || "."; > > This is too clever. Please use an if statement here. ok > > + // Ignore any occurrence of "\\: immediately before that one > typo oops > > + start = (this.winGetDrive(path) || "").length; > too clever. if please > > + for each(let i in arguments) { > > s/i/segment, component, whatever you like/ i implies integer-like thing Good idea. Even if we are not writing in Fortran :) > > + let result = paths.join("\\"); > + if (absolute) { > + result = "\\" + result; > + } > I'm not a fan of prepending strings. Instead of a prepend, do an append..ie > add drive letter first, result after. > > Same with if(root) below. Ok. Out of curiosity, what is wrong with prepending strings? > > + winGetDrive: function winHasDrive(path) { <-- that's bad > Name indicates boolean, return is a string/null. call it winGetDrive Sorry, typo. As you see, field name is already winGetDrive. > > similar thing with winIsAbsolute -> winEnsureAbsolute This one, I don't understand. The method returns a boolean, why should I call it "ensure"?

> > > > > + let result = paths.join("\\"); > > + if (absolute) { > > + result = "\\" + result; > > + } > > I'm not a fan of prepending strings. Instead of a prepend, do an append..ie > > add drive letter first, result after. > > > > Same with if(root) below. > > Ok. Out of curiosity, what is wrong with prepending strings? Nothing. It's slightly more straightforward to assemble the string in the order that it ends up. > > > > > + winGetDrive: function winHasDrive(path) { <-- that's bad > > Name indicates boolean, return is a string/null. call it winGetDrive > > Sorry, typo. As you see, field name is already winGetDrive. Right :) > > > > > similar thing with winIsAbsolute -> winEnsureAbsolute > > This one, I don't understand. The method returns a boolean, why should I > call it "ensure"? Sorry, misread. I seem to be suffering a bout of dyslexia today.

Applied the requested back-end changes.

David, I likely wont be able to get to these reviews this week. Either ask someone else for review or hold tight :)

Comment on attachment 634623 [details] [diff] [review] Unix test suite Review of attachment 634623 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js @@ +5,5 @@ > dump("WORKER "+text+"\n"); > } > > function send(message) { > +// log("Sending " + JSON.stringify(message) + "errno: " + ctypes.errno); just remove it, right? @@ +208,5 @@ > +function test_path() > +{ > + ok(true, "test_path: starting"); > + is(OS.Unix.Path.basename("a/b"), "b", "basename of a/b"); > + is(OS.Unix.Path.basename("a/b/"), "", "basename of a/b/"); is basename really "" for b/? Shouldn't it be 'b'?

(In reply to Doug Turner (:dougt) from comment #32) > Comment on attachment 634623 [details] [diff] [review] > Unix test suite > > Review of attachment 634623 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js > @@ +5,5 @@ > > dump("WORKER "+text+"\n"); > > } > > > > function send(message) { > > +// log("Sending " + JSON.stringify(message) + "errno: " + ctypes.errno); > > just remove it, right? > > @@ +208,5 @@ > > +function test_path() > > +{ > > + ok(true, "test_path: starting"); > > + is(OS.Unix.Path.basename("a/b"), "b", "basename of a/b"); > > + is(OS.Unix.Path.basename("a/b/"), "", "basename of a/b/"); > > is basename really "" for b/? Shouldn't it be 'b'? Interestingly, this depends on whether we follow Unix conventions ('b') or Python conventions (""). I have followed the second here, and I should probably document this.

Comment on attachment 635717 [details] [diff] [review] Windows back-end stripBackslashes -> trimBackslashes might be better ensureNotUNC should probably ensure that the path has a drive: component so functions like basename do not throw string exceptions + /** + * Return |true| if the path is absolute, |false| otherwise. + * + * We consider that a path is absolute if it starts with "\\" + * or "driveletter:\\". + */ That seems wrong. paths that start with \ are relative to current drive. That's in no way absolute. Please address above comments in a followup patch.

(In reply to Taras Glek (:taras) from comment #34) > Comment on attachment 635717 [details] [diff] [review] > Windows back-end > > stripBackslashes -> trimBackslashes might be better ok > ensureNotUNC should probably ensure that the path has a drive: component so > functions like basename do not throw string exceptions Well, none of the functions throws if there is no drive component, so this does not really seem useful. Do you really want that? I believe that this will only make |basename| and |dirname| less useful. > > > + /** > + * Return |true| if the path is absolute, |false| otherwise. > + * > + * We consider that a path is absolute if it starts with "\\" > + * or "driveletter:\\". > + */ > > That seems wrong. paths that start with \ are relative to current drive. > That's in no way absolute. Well, I am sticking with the definition of MSDN: « A file name is relative to the current directory if it does not begin with one of the following: [...] A single backslash, for example, "\directory" or "\file.txt". This is also referred to as an absolute path. » Are you sure that you want me to change this? > Please address above comments in a followup patch. Ok.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ff738074a6d https://hg.mozilla.org/integration/mozilla-inbound/rev/12964e66b3b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/992ba16d7d5e https://hg.mozilla.org/integration/mozilla-inbound/rev/82e16d73fe09

https://hg.mozilla.org/mozilla-central/rev/4ff738074a6d https://hg.mozilla.org/mozilla-central/rev/12964e66b3b3 https://hg.mozilla.org/mozilla-central/rev/992ba16d7d5e https://hg.mozilla.org/mozilla-central/rev/82e16d73fe09