Closed
Bug 857077
Opened 11 years ago
Closed 11 years ago
[OS.File] Add an option ignoreAbsent to OS.File.remove
Categories
(Toolkit Graveyard :: OS.File, defect)
Toolkit Graveyard
OS.File
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla25
People
(Reporter: Yoric, Assigned: poiru)
Details
(Whiteboard: [mentor=Yoric][lang=js])
Attachments
(1 file, 2 obsolete files)
4.46 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
With OS.File, the following idiom is very common: try { OS.File.remove(somePath); } catch (ex if ex.becauseNotExists) { // ignore } Given that this idiom is both common and rather annoying to implement in asynchronous mode, I believe that we could just as well add this as an option to OS.File.remove. e.g. OS.File.remove(somePath, { ignoreAbsent: true });
Comment 1•11 years ago
|
||
I wants to work on this bug. I have prerequisite knowledge of c,c++,javascript, Html and Css. So please assign this to me.
Updated•11 years ago
|
Flags: needinfo?(dteller)
Comment 2•11 years ago
|
||
I would have liked to work on this bug too but since Rahul asked for it first you can let him take it if he is keen on it or else i would like to take it up!
Reporter | ||
Comment 3•11 years ago
|
||
Rahul, if you want this bug, you can have it. Prasanth, I'm sure we'll be able to find something else for you :) Rahul, you can find the source code of |OS.File.remove| here: http://dxr.mozilla.org/mozilla-central/toolkit/components/osfile/osfile_win_front.jsm#l339 (Windows version) and http://dxr.mozilla.org/mozilla-central/toolkit/components/osfile/osfile_unix_front.jsm#l277 (Unix version) For a first version, you can just add the relevant |try { ... } catch () { ... }| block (see comment 0) to both functions. We will improve gradually from there.
Flags: needinfo?(dteller)
Comment 4•11 years ago
|
||
David sir, yes i wants to work on this bug.
Reporter | ||
Comment 5•11 years ago
|
||
Then, it is yours :) Do not hesitate to ask questions here or over IRC (irc.mozilla.org, channel #introduction). I will be a bit difficult to reach during the next few days, but I will do my best to help you.
Assignee: nobody → rahulgandhi38
Comment 6•11 years ago
|
||
Thanks. Sir i m new to development. so pls help me. I wants to ask if we have to implement this try catch statement only.
Reporter | ||
Comment 7•11 years ago
|
||
Well, as I mentioned, for a first prototype, just implementing this try/catch statement will be good. Once this is done, we will decide whether we wish to optimize further.
Updated•11 years ago
|
Assignee: rahulgandhi38 → nobody
Updated•11 years ago
|
Flags: needinfo?(rahulgandhi38) → needinfo?
Reporter | ||
Comment 9•11 years ago
|
||
If I understand correctly, Rahul has abandoned that bug. Prasanth, do you want to take it?
Flags: needinfo? → needinfo?(prasanth_b4u)
Reporter | ||
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 10•11 years ago
|
||
Hello Sir ! I would like to work on this bug, with ur consent !!!
Comment 11•11 years ago
|
||
Hi David... I will be gone for a week or two more... If Sajit would like to work on this bug you can assign it to him.
Flags: needinfo?(prasanth_b4u)
Assignee | ||
Comment 12•11 years ago
|
||
Since this bug has been dormant for a month, I went ahead and created a patch. I also added some tests.
Attachment #766286 -
Flags: review?(dteller)
Reporter | ||
Comment 13•11 years ago
|
||
Comment on attachment 766286 [details] [diff] [review] Patch Review of attachment 766286 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks for the patch! If you're not familiar with the rules for checking in code, the next step is reading this: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Attachment #766286 -
Flags: review?(dteller) → review+
Reporter | ||
Comment 14•11 years ago
|
||
Actually, as suggested by Taras, it would be a good idea to make this the default behavior, i.e. only throw if ignoreAbsent is (explicitly) set to something falsey.
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #14) > Actually, as suggested by Taras, it would be a good idea to make this the > default behavior, i.e. only throw if ignoreAbsent is (explicitly) set to > something falsey. Would you prefer keeping the option name ignoreAbsent or should we change it to e.g. noIgnoreAbsent or throwIfAbsent? (In reply to David Rajchenbach Teller [:Yoric] from comment #13) > If you're not familiar with the rules for checking in code, the next step is > reading this: > http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed Thanks! I presume I need to be assigned this bug to (eventually) add the checkin-needed keyword?
Reporter | ||
Comment 16•11 years ago
|
||
(In reply to Birunthan Mohanathas [:poiru] from comment #15) > (In reply to David Rajchenbach Teller [:Yoric] from comment #14) > > Actually, as suggested by Taras, it would be a good idea to make this the > > default behavior, i.e. only throw if ignoreAbsent is (explicitly) set to > > something falsey. > > Would you prefer keeping the option name ignoreAbsent or should we change it > to e.g. noIgnoreAbsent or throwIfAbsent? Let's keep |ignoreAbsent|. > > (In reply to David Rajchenbach Teller [:Yoric] from comment #13) > > If you're not familiar with the rules for checking in code, the next step is > > reading this: > > http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed > > Thanks! I presume I need to be assigned this bug to (eventually) add the > checkin-needed keyword? Actually, I'm not sure. But I just assigned you, just in case.
Assignee: nobody → birunthan
Assignee | ||
Comment 17•11 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #16) > Let's keep |ignoreAbsent|. I've attached the updated patch. > Actually, I'm not sure. But I just assigned you, just in case. Thanks, that seems to have done the trick.
Attachment #766286 -
Attachment is obsolete: true
Attachment #766477 -
Flags: review?(dteller)
Reporter | ||
Comment 18•11 years ago
|
||
Comment on attachment 766477 [details] [diff] [review] Patch v2 Review of attachment 766477 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Don't forget to add ";r=yoric"at the end of your commit message. ::: toolkit/components/osfile/osfile_unix_front.jsm @@ +269,5 @@ > * > * @param {string} path The name of the file. > + * @param {*=} options Additional options. > + * - {bool} ignoreAbsent If |true|, do not fail if the file > + * does not exist. |true| by default. I would write: "if |false|, throw an error if the file does not exist. |true| by default."
Attachment #766477 -
Flags: review?(dteller) → review+
Assignee | ||
Comment 19•11 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #18) > I would write: "if |false|, throw an error if the file does not exist. > |true| by default." Updated the comment.
Attachment #766477 -
Attachment is obsolete: true
Attachment #767227 -
Flags: review?(dteller)
Reporter | ||
Comment 20•11 years ago
|
||
Comment on attachment 767227 [details] [diff] [review] Patch v3 Review of attachment 767227 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks.
Attachment #767227 -
Flags: review?(dteller) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 21•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4b6314f4fe
Flags: in-testsuite+
Keywords: checkin-needed
Comment 22•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8b4b6314f4fe
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Updated•11 months ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•