Re: Datatype interface for RELAX NG (updated)

[Kohsuke KAWAGUCHI <kohsuke.kawaguchi@eng.sun.com>]

Changes are incorporated and the updated binary is attached. I'm sorry
to hasten you but I want them to be fixed as soon as possible.


> 1.  Since its datatypeLibrary not dataTypelibrary, the names of classes
> should use Datatype not DataType.
>
> 2. I don't think Datatype.check() needs to be optional.  If implementations
> can't provide detailed information about why a match fails they can just
> throw an uninformayive DatatypeException.
>
> 3. The protected members of DataTypeException are evil. Make them private
> and final and add accessors.
> 
> 4. Add a constructor with no arguments to DataTypeException to deal with 2.
> 
> 5. I don't think the content member of DataTypeException is necessary.  The
> caller knows this already.

Done.


> 6. displayName on DataType is not necessary.  The caller knows what the name
> of the datatype is, since they used the name to create it with
> createDataTypeBuilder or getType.  Conceptually, the datatype library is a
> mapping from local names to datatypes.  The name isn't part of the datatype.
> The object representing a <data> pattern should store the name in addition
> to the DataType object.

I still have a sentiment to it, but probably you are right.

> 7. Return value of characters on DataTypeStreamingValidator is unclear.  For
> example, if we have a streaming validator for int, and the first call to
> characters passes "+", what does characters() return?  It should return true
> because it no error has been detected. isValid() should be called only when
> you have finished accumulating the value.  Thus characters() does not return
> what isValid would return after processing the fragment.  For this reason, I
> think isValid() should be renamed to something like finish() or end().

Done.

> 8. How does DatatypeStreamingValidator provide the diagnosis information
> that you get from the DatatypeException? This should be possible, index in
> DatatypeException should be an index into the entire string.

Added the check method. Any suggestion for a better name?

> 9. Get rid of reset() on DatatypeStreamingValidator.  It's an
> over-optimization and complicates the semantics of the object.

I modified the semantics of the DatatypeStreamingValidator, and allow
the finish method and the check method to be called only once. This
should give the datatype library a chance to reuse the streaming
validator objects, if that is necessary.

> 10. On ValidationContext, I think if we support isUnparsedEntity(), we
> should also support isNotation(). (SAX supports both.)

Done. But no known datatype library uses this method.


> 11. We need to think about how instances of DatatypeLibrary get created.
> Ideally, what we want is that an implementor of a DatatypeLibrary provides

Added a DatatypeLibraryFactory class to the helper package. Since this
class relies on a non-standard class "sun.misc.Service", I think it's
not a bad idea to make it an optional "utility" class.

This class has protected "create" method, which should be implemented by
the datatype library to create a DatatypeLibrary object from an URI.

It also has the find method, which enumerates available DatatypeFactory
implementations and call its create method to see if it supports the
specified URI.

> 12. DataType needs a valueHashCode() method consistent with sameValue() in
> order to implement keys.

Done. I guess a custom implementation of Set would also help...



regards,
--
Kohsuke KAWAGUCHI                          +1 650 786 0721
Sun Microsystems                   kohsuke.kawaguchi@sun.com

relaxng.zip