castor

Various remarks from the IRC logs:

<ralf> and NMTOKENS and IDREFS should be separated from XSList class
<ralf> i've see some code to distinguish between those by looking into the component type
<ralf> i suggest to also create issues to track this
<wguttmn> what class do you refer to when talking about IDREFS ?
<ralf> i will search for it as i do not remember where it was. just recognized it at the cleanup session
<wguttmn> it's in XSList, which in its getName() method does some check what to return, based upon the content type ....
<ralf> e.g. lines 546-570 of DescriptorSourceFactory
<wguttmn> iow, found it ....

...

<ralf> it think it would be better if the code at the mentioned lines would be created by XSList, XSNMTokens and XSIdRefs

...

<wguttmn> and XSNMTokens and XSIdRefs could still extend XSList ....
<ralf> i would move soem more code from XSList into XSListType and make XSNMTokens and XSIdRefs extend XSListType
<ralf> which is the abstract base class for all list types
<wguttmn> oh, i have not seen that yet ....
<wguttmn> will have a look ....
<wguttmn> it's TypeConversion, lines 201ff that actually create those XSList instances with content type set to either NMToken or IDREF ... funny ....
<wguttmn> that shoudl be easy to change ....
<wguttmn> do we have an issue for this already ?
<ralf> one could in addition try to move more of the validationCode() methods into the abstract facet classes
<ralf> @XSList: no
<ralf> not that i know of
<wguttmn> okay, will create one later on ....
<ralf> before my refactoring this task had not been possible as we had XSList as well as XSList2 and XSListODMG extending it

Initial patch for review. Problem is that it breaks about 20 test of the CTF, and I honestly don't have a clue as to why .. ;-(.

Your patch does not contain the two new classes.

Now it does .. .

The tests are failing because validation code is not being generated for lists.

I have been thinking along the same way ... but rules things out one after one. It cannot be a test against instanceof XSList, as this will return correct results for XSNMTokens and XSIdrefs as well. Iow, what else could it be ?

look at a before and after of

build/tests/output/MasterTestSuite/sourcegenerator/Facets/UnwrappedNumbers//NumberTestsDescriptor.java

and look (for example) for _mydecimal and you'll see that validation code is not being generated for collections.

Can you connect to IRC ? I just had a look at DescriptorSourceFactory.validationCode() , and I cannot see how it should have been generatd pre-patch ...

Why do XSNMTokens and XSIdRefs extend XSList instead of XSListType?

You could have moved jType and its construction to XSListType or just use Vector as fixed collection types at XSNMTokens and XSIdRefs.

Yes, I will do later on. But right now I'd rather focus on the wrong results of the initial refactoring ... as the code in DescriptorResourceFactory.validationCode() is not easy to refactor.

Updated patch that resolves the problems mentioned above. All tests finish successfully. I still need to make XSNMTokens and XSIdrefs extend XSListType, though.

Or maybe I shall address this as part of a follow-up tasks ...

If you feel more comfortable with the previous code I can also revert my refactoring patch so you don't need to introduce new compiler warnings and go back to unreadable code step by step just as you are to lazy to spend 5 more minutes to clean up your code. You only need to send me a note and I can go ahead. And belief me, I was just about to do that without any previous note when I first read your last comment.

If that issue would be about a serious bug I could understand to fix that as soon as possible irrespective of how the solution looks like but I don't see any reason to hurry here.

Ralf, there's only one reason why I mentioned moving things to a follow-up task: the desire not to block a particular code area for days to come. As a matter of fact, I am not happy at all about the patch, as I had to introduce code to CollectionInfo (if I remember correctly) that I do not like at all.

In other words, whilst the patch removes all the conditionals from XSList (by introducing XSNMTokens and XSIdRefs), it makes things worse in another area. Rather than pondering fro days about a better solution (that in my opinion would require a lot of refactoring), I was thinking about releasing a partial improvement earlier (allowing you and Eddie to merge changes in as they become available, and as a result be able to touch other code areas), and subsequently discuss the 'botch' introduced to CollectionInfo. As you have said yourself, we are not talking about a blocker, but about general refactoring to improve the code base.

In addition, I am rather in favor of making a patch ready for review early (to, again, get your feedback, e.g. about the approach taken), especially when knowing that I am about to leave office and that I won't be on-line for the remainder of the evening.

Updated patch for review; this time, the new classes XSNMTokens and XSIdrefs extend from XSListType, and in addition I have introduced a new class XSCollectionFactory fro creation of XSListType instances.

This time with file attached ....

Looks good to me.

I am still not contended with this patch as it increases the number of warnings in types package by a factor of 6 (in words six) including 2 warnings. I therefore reopen this issue for rework.

Alright, added the one missing Javadoc. With regards to all the remaining issues, I cannot remember that we (as committers) agreed that methods should be declared 'final' just because there's a compiler 'info' element. I do remember having to remove some 'final' declarations recently, and everytime I have been asking me why it was declared as such initially. I hope you agree that there's no such things as a common view on the use of 'final' other than on method parameters. Having said that, I am more than open to have this discussion .. if there's a desire.

And why on earth should I declare the constructor of XSCollectionFactory as static (other than to please Eclipse). It's a utility class, true. It has (so far) one static method only. But why should I prevent anybody from creating an instance of this class (if (s)he has a desire to do so) ?

Having said that, I wish I could have spent my time on looking e.g. at

http://jira.codehaus.org/browse/CASTOR-1840
http://jira.codehaus.org/browse/CASTOR-1839
http://jira.codehaus.org/browse/CASTOR-1838
http://jira.codehaus.org/browse/CASTOR-1830

and others .... where there's either true regression issues or at least inconsistencies introduced between various Castor versions.

And finally, I had already picked up that missing Javadoc as part of another issue, as I have noticed the warning on this class.

Regarding "final" on classes and methods, I agree with Werner. While I agree that marking everything under the sun as "final" is safer, it is also more limiting.

My intention has always been to use final on methods where it is my sincere intention to convey that this method is final by design, and in my opinion the majority of methods are not final per definition.

Ralf, feel free to close this at your convenience .... as it's been you who has re-opened it.

If that makes you happy I'll close the issue. Having said that I do not agree and take the consequence.

I'd appreciate if we stopped declaring methods 'final' that are assumed to be final by design. I am just looking at the Castor Maven plugin together with the Mojo guys from Codehaus, and within the GenerateMojo in this project, the have actually overloaded various methods of SourceGenerator (which is a valid approach).

With a recent commit set, apparently all setter methods of SourceGenerator have been declared final. Honestly, a) I don't think that this is actually correct, and b) I find it quite a nuisance to selectively revert those (unnecessary) changes again and again, rebuild, redeploy a JAR file to a Maven repo, etc.

In other words, let's please not use the final attribute on methods unless this reflects a design decision. I can point you to numerous discussions on the usefulness of some checkstyle rules, and the overambitious use of final definitely is part of this.