Closed lukasj closed 7 months ago
OK, great, will do.
Also, @sebersole, since you're more fluent in the XML schema, it would be great if you could take a look too.
Not sure I'd say I'm more fluent, just been doing a lot of XSD/JAXB lately ;)
The changes looked fine. A good chunk are just documentation changes and adding a new "options" value for various database objects.
Great, thanks.
A few minor things I did notice -
<table/>
, <secondary-table/>
, <join-table/>
, ... into a group.(1) and (2) because those might be long/complex. And a side benefit being that it helps with (3).
The argument for (3) is just that, e.g., it would have made much of this change simpler. You could have added table-related check, comment and options in a single place - the group. E.g. from Hibernate's XSD -
<xsd:group name="common-table-group">
<xsd:annotation>
<xsd:documentation>
See `@org.hibernate.annotations.Comment`
See `@org.hibernate.annotations.Check`
</xsd:documentation>
</xsd:annotation>
<xsd:sequence>
<xsd:element name="check" type="orm:check-constraint" minOccurs="0" />
<xsd:element name="options" type="xsd:string" minOccurs="0" />
<xsd:element name="comment" type="xsd:string" minOccurs="0" />
</xsd:sequence>
</xsd:group>
<xsd:complexType name="table">
...
<xsd:sequence>
<xsd:group ref="common-table-group" />
<xsd:element name="unique-constraint" type="orm:unique-constraint" minOccurs="0" maxOccurs="unbounded"/>
<xsd:element name="index" type="orm:index" minOccurs="0" maxOccurs="unbounded"/>
...
</xsd:sequence>
...
<xsd:complexType/>
With the group, you'd make that change once in one spot.
I suggest making comment an element, whereas it is currently defined as an attribute.
That sounds good to me. WDYT, @lukasj ?
Maybe options as well?
This one I don't care about. I think the idea is that it should be something short that fits in an attribute.
To help with any on-going maintenance, I'd consider better use of XSD groups to extract common stuff.
That's a very good idea, but it's something we could open a separate issue for.
Maybe options as well?
This one I don't care about. I think the idea is that it should be something short that fits in an attribute
It's about intention and XSD capabilities. E.g., for a column do you see column-definition and options as mutually exclusive? It would seem like they are. But at the moment, you cannot enforce that in the XSD. And unfortunately you won't be able to because column-definition is already an attribute. In XSD you can only force a choice between elements.
Just suggesting we might want to be proactive about that
for a column do you see column-definition and options as mutually exclusive?
We did not say that. Right now you can use them together. But I agree that's not very useful and in principle we could make them mutually exclusive.
I don't have an opinion either way.
I'll move comment
to become an element
I changed comment
from an attribute to an element. For options
, I kept it as an attribute for now (no strong opinion here, I'm only thinking that this is going to be used for something short). For the last item, I filed #546 as I think that it deserves separate PR
I'm only thinking that this is going to be used for something short
Yeah, I think that's most likely.
see #458
@gavinking review this one carefully - I have also added few missing things from the past (mostly related to foreign-key added in 2.1)
diff -ub orm_3_1.xsd orm_3_2.xsd
``` --- orm_3_1.xsd 2023-10-27 22:59:49 +++ orm_3_2.xsd 2023-10-27 23:05:22 @@ -1,6 +1,6 @@ +