857077 - [OS.File] Add an option ignoreAbsent to OS.File.remove

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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]

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]

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] - still alive but not very active]

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]

David sir, 
yes i wants to work on this bug.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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]

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] - still alive but not very active]

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] - still alive but not very active]

Rahul, any news?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

If I understand correctly, Rahul has abandoned that bug.
Prasanth, do you want to take it?

Comment 10

[Sajit Menon]

Hello Sir !
I would like to work on this bug, with ur consent !!!

Comment 11

[Prasanth Balakrishnan]

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

[poiru]

Attachment: 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] - still alive but not very active]

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] - still alive but not very active]

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

[poiru]

(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] - still alive but not very active]

(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

[poiru]

Attachment: 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] - still alive but not very active]

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

[poiru]

Attachment: 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] - still alive but not very active]

Comment on attachment 767227 [details] [diff] [review]
Patch v3

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

Looks good, thanks.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4b6314f4fe

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8b4b6314f4fe