The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla25

Status

()

Toolkit
OS.File
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Yoric, Assigned: poiru)

Tracking

Trunk
mozilla25
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

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

4 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

4 years ago
Flags: needinfo?(dteller)
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!
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

4 years ago
David sir, 
yes i wants to work on this bug.
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

4 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.
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.
Rahul, any news?
Flags: needinfo?(rahulgandhi38)

Updated

4 years ago
Assignee: rahulgandhi38 → nobody

Updated

4 years ago
Flags: needinfo?(rahulgandhi38) → needinfo?
If I understand correctly, Rahul has abandoned that bug.
Prasanth, do you want to take it?
Flags: needinfo? → needinfo?(prasanth_b4u)
OS: Mac OS X → All
Hardware: x86 → All

Comment 10

4 years ago
Hello Sir !
I would like to work on this bug, with ur consent !!!
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

4 years ago
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.
Attachment #766286 - Flags: review?(dteller)
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+
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

4 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?
(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

4 years ago
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.
Attachment #766286 - Attachment is obsolete: true
Attachment #766477 - Flags: review?(dteller)
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

4 years ago
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.
Attachment #766477 - Attachment is obsolete: true
Attachment #767227 - Flags: review?(dteller)
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

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4b6314f4fe
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8b4b6314f4fe
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.