Last Comment Bug 857077 - [OS.File] Add an option ignoreAbsent to OS.File.remove
: [OS.File] Add an option ignoreAbsent to OS.File.remove
Status: RESOLVED FIXED
[mentor=Yoric][lang=js]
:
Product: Toolkit
Classification: Components
Component: OS.File (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Birunthan Mohanathas [:poiru]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-02 07:16 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2013-07-02 12:48 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.26 KB, patch)
2013-06-22 05:59 PDT, Birunthan Mohanathas [:poiru]
dteller: review+
Details | Diff | Splinter Review
Patch v2 (4.44 KB, patch)
2013-06-23 13:08 PDT, Birunthan Mohanathas [:poiru]
dteller: review+
Details | Diff | Splinter Review
Patch v3 (4.46 KB, patch)
2013-06-25 08:05 PDT, Birunthan Mohanathas [:poiru]
dteller: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2013-04-02 07:16:08 PDT
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 Rahul Gandhi 2013-04-07 11:56:30 PDT
I wants to work on this bug. I have prerequisite knowledge of  c,c++,javascript, Html and Css.
So please assign this to me.
Comment 2 Prasanth Balakrishnan 2013-04-11 13:18:28 PDT
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!
Comment 3 David Teller [:Yoric] (please use "needinfo") 2013-04-12 01:42:03 PDT
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.
Comment 4 Rahul Gandhi 2013-04-12 02:02:44 PDT
David sir, 
yes i wants to work on this bug.
Comment 5 David Teller [:Yoric] (please use "needinfo") 2013-04-12 02:04:07 PDT
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.
Comment 6 Rahul Gandhi 2013-04-12 02:06:45 PDT
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.
Comment 7 David Teller [:Yoric] (please use "needinfo") 2013-04-15 01:47:02 PDT
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.
Comment 8 David Teller [:Yoric] (please use "needinfo") 2013-04-25 08:22:47 PDT
Rahul, any news?
Comment 9 David Teller [:Yoric] (please use "needinfo") 2013-05-05 04:54:54 PDT
If I understand correctly, Rahul has abandoned that bug.
Prasanth, do you want to take it?
Comment 10 Sajit Menon 2013-05-20 05:32:18 PDT
Hello Sir !
I would like to work on this bug, with ur consent !!!
Comment 11 Prasanth Balakrishnan 2013-05-23 10:53:42 PDT
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.
Comment 12 Birunthan Mohanathas [:poiru] 2013-06-22 05:59:52 PDT
Created attachment 766286 [details] [diff] [review]
Patch

Since this bug has been dormant for a month, I went ahead and created a patch. I also added some tests.
Comment 13 David Teller [:Yoric] (please use "needinfo") 2013-06-22 12:32:39 PDT
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
Comment 14 David Teller [:Yoric] (please use "needinfo") 2013-06-22 13:17:32 PDT
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.
Comment 15 Birunthan Mohanathas [:poiru] 2013-06-23 03:37:39 PDT
(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?
Comment 16 David Teller [:Yoric] (please use "needinfo") 2013-06-23 12:04:18 PDT
(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.
Comment 17 Birunthan Mohanathas [:poiru] 2013-06-23 13:08:19 PDT
Created attachment 766477 [details] [diff] [review]
Patch v2

(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.
Comment 18 David Teller [:Yoric] (please use "needinfo") 2013-06-25 02:37:42 PDT
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."
Comment 19 Birunthan Mohanathas [:poiru] 2013-06-25 08:05:51 PDT
Created attachment 767227 [details] [diff] [review]
Patch v3

(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.
Comment 20 David Teller [:Yoric] (please use "needinfo") 2013-06-26 10:09:04 PDT
Comment on attachment 767227 [details] [diff] [review]
Patch v3

Review of attachment 767227 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-07-01 06:32:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4b6314f4fe
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-07-02 12:48:24 PDT
https://hg.mozilla.org/mozilla-central/rev/8b4b6314f4fe

Note You need to log in before you can comment on or make changes to this bug.