Have SourceGenerator use a separate packages for class descriptor classes

Accepting.

Here's an idea for the command-line options:

Usage: -i filename [-package package-name] [-descpackage package-name]
[-subpackage package-name] [-dest dest-dir] [-line-separator ( unix | mac |
win)] [-f ] [-h ] [-verbose ] [-nodesc ] [-types types] [-type-factory
classname] [-nomarshall ] [-testable ] [-sax1 ] [-binding-file filename]

-package = for types
-subpackage = for subclasses of type
-descpackage = for class descriptors

However, it might be better to just put the subclasses and descriptors in
relative packages automatically:

So if the user specifies:

-package com.mycompany.myproject

It puts the classes in com.mycompany.myproject, the types in
com.mycompany.myproject.types, the descriptors in com.mycompany.myproject.desc,
and the subclasses in com.mycompany.myproject.model (or whatever)

Robert, can you please be a bit more specific as to what you would liek to see implemented

Can I get my 'peer committers' to vote on this ? In other words, shall we make new command line options/setters available, and use these (and if nothing is specified, fall back onto what has been sugested above) ?

Sounds like a reasonable request, and just putting descriptors in .desc relative to the source should be adaquate. But I am not sure what is meant by "subclasses" in this context.

One side benefit of putting all of the class descriptor files in a separate package is that it would be easy in ANT to only run the source generator when the XSD file is newer than the generated source (aka, newer than the descriptors). That way, when the generated source shares are directory with non-generated source, you can still tell when the source generator was run. (Assuming that you don't put anything in .desc)

Sounds like a good idea to me too.

Since I don't understand what "make a separate package for subclasses" is suggesting, how about we just move the descriptors into a separate package, and do this always and not as a result of a command-line switch? That is, no new command line options, and all current *Descriptor.java go into a new .desc package. Users wouldn't be doing anything with those classes anyway, right? Those are purely internal?

If there's agreement on this then I'll make it happen.

I have this working. There's one wrinkle – you cannot import from the "default package". The solution is that source generated into the default package does not get separated this way, but code generated into a normal package will get a descriptor subdirectory.

However, I'm not sure how to tell that the generated descriptors are being used as opposed to some default descriptors. How does Castor XML autodiscover the descriptors, since the generated classes have no reference to them?

Here's how it works .. have a look at XMLClassDescriptorResolverImpl.resolve(Class) which shows you how Castor established descriptors for classes. If there's a global mapping, a <class> mapping will be transformed into a XML ClassDescriptor by the MappingLoader. Same for package-based .castor.xml files. Otherwise, it will look (within the same package) if there's a *Descriptor class available for a class X. If not, the Introspector will be used to establish a 'default' descriptor using reflection. Does this help ?

Attaching a first patch for review & comment. Changes are:

1) Move all descriptor-handling code to a new package.
2) Start collecting builder constants in a single class for constants, so we don't have copies of the same constants all over builder.
3) Descriptors always go into a new package, no configuration available – unless code is being generated into the default package where it is simply not possible to put the descriptor code into a different package. Code from the "default package" cannot be imported nor referenced in any other way from code in any other package.
4) In javasource there were a couple copies of static methods to take a fully-qualified java class name and get the package from it. I moved all of those into JNaming (seems an appropriate place for it).

The correct new descriptor location is written into the .castor.cdr – which is how they were being located by XML during use.

I think this is ready to commit, but I wanted feedback before committing this sort of functional change.

> The correct new descriptor location is written into the .castor.cdr - which is how they were being located by
> XML during use.
Without having reviewed the code yet (pending .. ), this statement is a bit ambiguous in the sense that the XMLClassDescriptoptorResolverImpl will look for such a file, and all all the descriptors found to its internal cache (and thus allow start using them). If such a file cannot be found, or if a descriptor is not enlisted within such a file (package-scoped), I think XMLClassDescriptoptorResolverImpl will try to hunt down a descriptor using some default algorithm. And I am just getting aware that I shall look this up myself ....

XMLClassDescriptorResolverImpl.loadDescriptorClass() is the method I had in mind. After the suggested change, this method will now have to load a Descriptor instance from the very same package as the domain class and from a sub-package named 'descriptors', right ?

Ah, OK, I thought maybe the .castor.cdr was sufficient, but I agree that updating the CDR to also look in a specially-named subdirectory would be even better. This means, however, that the constant that names the subdirectory name should be owned by Castor-XML and not by Castor-XML-srcgen. It also means that we don't want people to have the ability to configure the name of this directory, so we should choose a name that people are unlikely to use for their own class files.

The name "descriptors" might be Good Enough. A longer package name such as castordescriptors seems unnecessary – but much less likely to confict with a directory that someone wants to use for their own code package.

Although finally, IMO, I think that generated code should usually be put into its own package and a separate package from code that is not generated, making code maintenance easier. Thus, as a general rule and best practice, I would recommend that anyone using the source generator choose a package that they are not using for other code for their generated code.

> This means, however, that the constant that names the subdirectory name should be
> owned by Castor-XML and not by Castor-XML-srcgen.
Yes.

> It also means that we don't want people to have the ability to configure the name of this directory,
Yes.

> The name "descriptors" might be good enough
Yes.

And finally, yes once more to your approach towards 'best practice'.

Attaching a 2nd patch which incorporates the structural changes made recently, moves some constants to XML, and handles missing .castor.cdr files gracefully by looking in the correct directory (after first looking in the previous location for these files, to handle older versions of Castor).

Tested by making the test suite delete the .castor.cdr files and verified that the generated descriptors were still loaded.

Change delivered. Should I put something in the release notes about the new location of the descriptor files?

Yes, please.