Bug 133911 - type.move() returns unclear exception "invalid destination"
Summary: type.move() returns unclear exception "invalid destination"
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.1.2   Edit
Hardware: All All
: P3 minor (vote)
Target Milestone: 3.6 M1   Edit
Assignee: Jay Arthanareeswaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-03-29 14:16 EST by Michael Pradel CLA
Modified: 2009-08-03 12:06 EDT (History)
3 users (show)

See Also:


Attachments
Patch with updated documentation (2.01 KB, patch)
2009-07-21 06:28 EDT, Jay Arthanareeswaran CLA
no flags Details | Diff
Updated patch (4.89 KB, patch)
2009-07-23 01:57 EDT, Jay Arthanareeswaran CLA
srikanth_sankaran: iplog+
srikanth_sankaran: review+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Pradel CLA 2006-03-29 14:16:44 EST
When calling move() on a SourceType with a valid destination, I get an exception complaining about an "invalid destination". This message should be more clear.

Code:
assertTrue(targetPackageFragment.exists());:
type.move(targetPackageFragment, null, null, true, null);
Comment 1 Adam Kiezun CLA 2006-03-29 15:01:32 EST
I think the destination is invalid. Types cannot be moved to packages.

The spec says nothing, however, about what the valid destinations for various elements are. So this bug is about the API.
(there's also the problem of not informative error message, but I guess good API comments should be priority here)
Comment 2 Olivier Thomann CLA 2009-06-25 14:49:37 EDT
Jay, could you please clarify the documentation inside:
org.eclipse.jdt.core.IJavaModel.move(IJavaElement[], IJavaElement[], IJavaElement[], String[], boolean, IProgressMonitor)
Comment 3 Jay Arthanareeswaran CLA 2009-06-29 01:36:07 EDT
Olivier,
Part of the documentation about the INVALID_DESTINATION is available in CopyElementsOperation#verify(IJavaElement). I was wondering if we can add a @see reference to this method. This is a protected method, by the way.
Comment 4 Jay Arthanareeswaran CLA 2009-07-21 06:28:55 EDT
Created attachment 142112 [details]
Patch with updated documentation

Updated the documentation relevant to INVALID_DESTINATION exception for the following methods:
(i) org.eclipse.jdt.core.IJavaModel.move(IJavaElement[], IJavaElement[], IJavaElement[], String[], boolean, IProgressMonitor)
(ii) org.eclipse.jdt.core.IJavaModel.copy(IJavaElement[], IJavaElement[], IJavaElement[], String[], boolean, IProgressMonitor)
Comment 5 Srikanth Sankaran CLA 2009-07-22 06:43:43 EDT
(In reply to comment #2)
> Jay, could you please clarify the documentation inside:
> org.eclipse.jdt.core.IJavaModel.move(IJavaElement[], IJavaElement[],
> IJavaElement[], String[], boolean, IProgressMonitor)

It would appear the concerned method is org.eclipse.jdt.core.ISourceManipulation.move(IJavaElement, IJavaElement, String, boolean, IProgressMonitor) and not 
org.eclipse.jdt.core.IJavaModel.move(IJavaElement[], IJavaElement[],
IJavaElement[], String[], boolean, IProgressMonitor)
though the latter could also benefit the additional clarification.

Jay, while we are at this, could you also fix the typo ("destinaion")
in the javadoc for 
org.eclipse.jdt.internal.core.CopyElementsOperation.verify(IJavaElement)

Does it make sense to add the clarification to the javadoc for org.eclipse.jdt.core.IJavaModelStatusConstants.INVALID_DESTINATION
and refer to it with a @see from ISourceManipulation,
IJavaModel ?

If we decide otherwise, does it make sense to add this clarification
to both move and copy methods instead referring one from the other ?



Comment 6 Olivier Thomann CLA 2009-07-22 07:00:38 EDT
(In reply to comment #5)
> Does it make sense to add the clarification to the javadoc for
> org.eclipse.jdt.core.IJavaModelStatusConstants.INVALID_DESTINATION
> and refer to it with a @see from ISourceManipulation,
> IJavaModel ?
Sounds good to me.

> If we decide otherwise, does it make sense to add this clarification
> to both move and copy methods instead referring one from the other ?
We could reference the constant in both methods. One explication is enough as long as we have one.
Comment 7 Jay Arthanareeswaran CLA 2009-07-23 01:57:47 EDT
Created attachment 142347 [details]
Updated patch

Moved the documentation to IJavaModelStatusConstants and added @see references in copy/move methods of IJavaModel and ISourceManipulation. Also corrected the typo in CopyElementsOperation's javadoc.
Comment 8 Srikanth Sankaran CLA 2009-07-23 04:07:01 EDT
Released Jay's clarifications to the documentation on HEAD for 3.6M1
Comment 9 Frederic Fusier CLA 2009-08-03 12:06:38 EDT
Verified for 3.6M1

IMO, I think it would have been better to separate the additional explanations from the first comment sentence using the <p></p> html tags, e.g.:
 * Status constant indicating that a destination provided for a copy/move/rename operation
 * is invalid.
 * <p>
 * The destination for a package fragment...