castor
  1. castor
  2. CASTOR-597

Have SourceGenerator use a separate packages for class descriptor classes

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.9.5.2
    • Fix Version/s: 1.1 M1
    • Component/s: XML code generator
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All
    • Bugzilla Id:
      1458
    • Number of attachments :
      2

      Description

      SourceGenerator should have an option to package both the subclasses and class
      description classes in seperate packages. This will make the documentation
      easier to navigate because it will be less cluttered.

      1. castor-597-first-patch.diff
        124 kB
        Edward Kuns
      2. castor-597-try2.diff
        134 kB
        Edward Kuns

        Activity

        Hide
        Keith Visco added a comment -

        Accepting.

        Show
        Keith Visco added a comment - Accepting.
        Hide
        Robert La Ferla added a comment -

        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)

        Show
        Robert La Ferla added a comment - 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)
        Hide
        Werner Guttmann added a comment -

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

        Show
        Werner Guttmann added a comment - Robert, can you please be a bit more specific as to what you would liek to see implemented
        Hide
        Werner Guttmann added a comment -

        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) ?

        Show
        Werner Guttmann added a comment - 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) ?
        Hide
        Edward Kuns added a comment -

        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)

        Show
        Edward Kuns added a comment - 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)
        Hide
        Ralf Joachim added a comment -

        Sounds like a good idea to me too.

        Show
        Ralf Joachim added a comment - Sounds like a good idea to me too.
        Hide
        Edward Kuns added a comment -

        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.

        Show
        Edward Kuns added a comment - 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.
        Hide
        Werner Guttmann added a comment -

        +1 for moving *Descriptor classes into a (new and separate) package named 'desc' or 'descriptors'. Wrt to sub-classes, I guess this refers to classes that are generated as a result of schema extension/restriction. Why one would see those to be moved into a separate package ... well, escapes me.

        Show
        Werner Guttmann added a comment - +1 for moving *Descriptor classes into a (new and separate) package named 'desc' or 'descriptors'. Wrt to sub-classes, I guess this refers to classes that are generated as a result of schema extension/restriction. Why one would see those to be moved into a separate package ... well, escapes me.
        Hide
        Edward Kuns added a comment -

        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?

        Show
        Edward Kuns added a comment - 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?
        Hide
        Werner Guttmann added a comment -

        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 ?

        Show
        Werner Guttmann added a comment - 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 ?
        Hide
        Edward Kuns added a comment -

        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.

        Show
        Edward Kuns added a comment - 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.
        Hide
        Werner Guttmann added a comment -

        > 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 ....

        Show
        Werner Guttmann added a comment - > 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 ....
        Hide
        Werner Guttmann added a comment -

        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 ?

        Show
        Werner Guttmann added a comment - 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 ?
        Hide
        Edward Kuns added a comment -

        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.

        Show
        Edward Kuns added a comment - 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.
        Hide
        Werner Guttmann added a comment -

        > 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'.

        Show
        Werner Guttmann added a comment - > 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'.
        Hide
        Edward Kuns added a comment -

        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.

        Show
        Edward Kuns added a comment - 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.
        Hide
        Edward Kuns added a comment -

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

        Show
        Edward Kuns added a comment - Change delivered. Should I put something in the release notes about the new location of the descriptor files?
        Hide
        Werner Guttmann added a comment -

        Yes, please.

        Show
        Werner Guttmann added a comment - Yes, please.

          People

          • Assignee:
            Edward Kuns
            Reporter:
            Robert La Ferla
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: