Community
Participate
Working Groups
+++ This bug was initially created as a clone of Bug #221211 +++ On bug 221211 comment 52, Rado said: "I disagree that we need the boolean return. This may be an use-case for deleteBatch but it is indeed an exceptional situation for delete(). Some client has requested a delete operation -- it can fail for many reasons and one particular reason is that the specified file doesn't exist, I consider that an exceptional situation. I suggest to add another exception (e.g. SystemFileNotFoundException) and catch it in deleteBatch." An exception is already prepared in SystemElmentNotFoundException. Do all committers agree that we want this breaking API change that late in the game? Who would want to take the bug?
I could implement the change if all committers agree to return void instead of boolean.
Ok, let's use Bugzilla Voting for deciding this. Here's some pros and cons: + making the change makes IFileService more consistent - we are past API Freeze already and the change is not absolutely necessary I'm undecided (voting "0" for now). Others please comment on the bug, or set a "review" flag to + or -. All committers are requested to vote.
Created attachment 101698 [details] Patch for this bug Attaching the patch for review. SftpFileService#delete() throws RemoteFileIOException instead of SystemElementNotFoundException if the remote file doesn't exist because it expects attrs to be null in this case which never happens. I see two options here: throw SENFE where the FIXME marker is or check for SSH_FX_NO_SUCH_FILE in makeSystemMessageException(). Martin, I leave this decision for you. I am going to commit the patch.
Marking as FIXED, the problem with SftpFileService can be tracked with bug 233651.
Thanks for applying this, Rado. Implementation is now in sync with the Javadocs. The one thing that I'm not 100% sure about is, whether we should really throw SystemElementNotFoundException in case the element(s) to delete are not found. I wrote some testcases, and found that Eclipse EFS IFileStore#delete() silently ignores this. What do you think? The other thing is that in (FTPService, LocalFileService, SftpService), I'm not so happy about the //$NON-NLS-1$ after SystemElementNotFoundException, because I'm not sure whether we wouldn't want to have the String ("delete") externalized eventually. I'd prefer not putting in the //$NON-NLS-1$ until it is decided that this String ("delete") really shouldn't ever be externalized. I'm reopening to sort out these discussion items, although I approve the patch.
Marking fixed since I'm sure that throwing Exceptions is the right thing to do, and the $NON-NLS-1$ tags with exceptions don't really hurt.
