Comment 1 Martin Aeschlimann 2007-07-02 08:29:10 EDT

Markus, can you fix this?

Comment 2 Markus Keller 2007-07-02 08:36:15 EDT

Will look into it.

Comment 3 Markus Keller 2007-07-03 06:52:16 EDT

This is a problem for all refactoring descriptors that set the arguments map directly by calling
JavaRefactoringDescriptor(String, String, String, String, Map, int). Their validateDescriptor() method validates the refactoring-specific typed fields, but those are all null in this case.

Karsten, what's the plan here? Where will these descriptors be validated?

Comment 4 Karsten Becker 2007-07-03 07:03:40 EDT

I'm already working on that. In fact I will create a patch later. The problem is that the fields are currently not used for reading. I'm rewriting those few ones that already have fields and initialize those in the constructor. Which leads to another problem.
It is for a contribution not allowed to return null on the createDescriptor stuff whith arguments and it is not allowed to throw an exception. So basically it is impossible to handle invalid argument maps. I rewrote it such that an IllegalArgumentException is thrown in such a case and check that it is handled carefully. At the moment it is not specified that a IllegalArgumentException could be thrown.

Comment 5 Markus Keller 2007-07-03 10:41:39 EDT

As I see it, RefactoringContribution.createDescriptor(..) should not do any checks other than the syntactic checks mentioned in the Javadoc. It should just create a RefactoringDescriptor whose validateDescriptor() will fail if the arguments were not OK.

I guess some of the checks in current validateDescriptor() methods can be dropped (e.g. checks for null or emptiness of new name string). We just have to make sure that a valid descriptor is able to create and initialize its refactoring. A successfully validated descriptor does not necessarily have to pass Refactoring#checkInitialConditions(..).

Comment 6 Karsten Becker 2007-07-03 11:19:38 EDT

The question is, what to do when an attribute is missing, or it's values are invalid. Like a "yes" in a attribute that is expected to be boolean. Imho it would be bad practice to accept it during creation of the descriptor as it might be non trivial to see where this error originated from if the yes would be stored, or even worse silently ignored.
Some checks like IJavaElement.exists() may be deffered to the validateDescriptor method. But an invalid Map should not be accepted.
Also consistency checks are not meant for construction time.
I'll now attach a patch that is work in progress and currently fails on the RenameTypeParameterTests because < is not allowed in a xml attribute...

Comment 7 Karsten Becker 2007-07-03 11:20:19 EDT

Created attachment 72961 [details]
Patch

Comment 8 Karsten Becker 2007-07-03 12:20:27 EDT

Created attachment 72964 [details]
Finalised patch

fixed the failing testcase for renameTypeParameterTest
At the moment a very extensive RefactoringDescriptor is enabled. Maybe it should not be enabled in the HEAD

Comment 9 Karsten Becker 2007-07-04 12:13:52 EDT

Created attachment 73038 [details]
Updated

This patch is an update for the IntroduceParameterObjectDescriptor to be less restrictive if the method is invalid in some way.

Comment 10 Karsten Becker 2007-07-09 08:47:16 EDT

Created attachment 73310 [details]
Final patch

Comment 11 Karsten Becker 2007-07-09 09:22:38 EDT

Created attachment 73314 [details]
IntroduceParameterObjectDescriptor update

Comment 12 Markus Keller 2007-07-10 02:47:26 EDT

Released patch 4 and 5 with a few changes:

IntroduceParameterObjectDescriptor.validateDescriptor(..) still failed when running IPO tests. Resolved by removing this check:
	if (!fMethod.exists()) {
		result.addFatalError("The method must exist"); //$NON-NLS-1$
		return result;
	}
This happens when the history service asks the descriptor to populate the argument map *after* the refactoring has been executed. At that time, fMethod indeed does not have to exist any more.

Also fixed problems in MoveDescriptor(..):
- read fMovePolicy from map (don't write it)
- fDestination can be ATTRIBUTE_DESTINATION or ATTRIBUTE_TARGET
- callers passed empty map and filled it only afterwards (see all IReorgPolicy.getDescriptor())

Did not release unused DescriptorMessages.RenameJavaElementDescriptor_reference_constraint.

TODO:
- All methods in JavaRefactoringDescriptorUtil need Javadoc, especially complex queries like getIntArray(Map map, String countAttribute, String arrayAttribute).
- Methods that throw IllegalArgumentException must declare the thrown exception and explain when that happens (in @throws).

- IPO tests still write to the log because RefactoringHistoryManager.checkArgumentMap(Map) is too restrictive. The mentioned rules (in RefactoringContribution, not RefactoringDescriptor) force map values to be non-empty, although empty value strings can make perfect sense for some usages (e.g. the name of the default package). Fix Javadocs and implementation in RefactoringContribution and all dependencies.

Comment 13 Karsten Becker 2007-07-10 11:09:05 EDT

Created attachment 73431 [details]
javadoc...

Comment 14 Markus Keller 2007-07-11 12:01:02 EDT

Released patch 'javadoc...' with all these fixes:

* IntroduceParameterObjectDescriptor:
- improved performance and robustness of createParameters(IMethod) by avoiding element access through IMethod#getParameterNames()
- restored readable if-else formatting in populateArgumentMap() (if-else always needs {}s)
- added 'throws IllegalArgumentException' where necessary

* JavaRefactoringDescriptorUtil
- fixed NLS warnings
- fixed messages (use ', not '')
- removed unsed getSelection(..) (the anonymous ISourceRange was a bad idea anyway)
- Retrieves *a* String/boolean (not 'an')

* RefactoringContribution:
- fixed Javadoc of createDescriptor(..) (value string can be empty)

* RefactoringHistoryManager:
- non-NLSd string
- last Status in checkArgumentMap(..) is bad

- RefactoringHistorySerializationTests.testWriteDescriptor8() failed