stevedlawrence commented on code in PR #1192:
URL: https://github.com/apache/daffodil/pull/1192#discussion_r1540258794
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -986,7 +993,7 @@ trait ElementBase
val pt = prim.primType
schemaDefinitionWhen(
(pt == PrimType.String || pt == PrimType.HexBinary) && lengthKind ==
LengthKind.Implicit,
- "Facets minLength and maxLength must be defined for type %s with
lengthKind='implicit'",
+ "The length, minLength and maxLength facets must be defined for type
%s with lengthKind='implicit'",
Review Comment:
This makes it sound like we need all three of length, minLength, and
maxLength. We should reword this to make it more clear you need
minLength/maxLength or length.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -963,20 +964,26 @@ trait ElementBase
/**
* Compute minLength and maxLength together to share error-checking
* and case dispatch that would otherwise have to be repeated.
+ *
+ * Also set them to the value of length, in the case we've used the length
facet
*/
final lazy val (minLength: java.math.BigDecimal, maxLength:
java.math.BigDecimal) =
computeMinMaxLength
- // TODO: why are we using java.math.BigDecimal, when scala has a much
- // nicer decimal class?
+
private val zeroBD = new java.math.BigDecimal(0)
private val unbBD = new java.math.BigDecimal(-1) // TODO: should this be a
tunable limit?
private def computeMinMaxLength: (java.math.BigDecimal,
java.math.BigDecimal) = {
schemaDefinitionUnless(
isSimpleType,
- "Facets minLength and maxLength are allowed only on types string and
hexBinary.",
+ "Facets length, minLength and maxLength are not allowed on complex
types",
)
typeDef match {
+ case st if hasRepType && hasLength => {
+ // if hasLength is true, then optRestriction is defined
+ val r = st.optRestriction.get
+ (r.lengthValue, r.lengthValue)
+ }
Review Comment:
If repType is defined then we need to get length information from the
repTypeElementDecl, not this element (i.e `r`). Since
repTypeElementDec.minLength and repTypeElementDecl.maxlength come from
computeMinMaxLength, and since that takes into account the length facet of the
rep type element, I *think* we can remove this new case and just rely on the
case below. That should should use the correct minLength/maxLength/length facet.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/ElementBase.scala:
##########
@@ -1026,13 +1034,15 @@ trait ElementBase
)
(r.minLengthValue, r.maxLengthValue)
}
- case (_, _, LengthKind.Implicit) =>
+ case (false, _, _, LengthKind.Implicit) =>
SDE(
"When lengthKind='implicit', both minLength and maxLength facets
must be specified.",
)
- case (false, true, _) => (zeroBD, r.maxLengthValue)
- case (false, false, _) => (zeroBD, unbBD)
- case (true, false, _) => (r.minLengthValue, unbBD)
+ case (false, false, true, _) => (zeroBD, r.maxLengthValue)
+ case (false, false, false, _) => (zeroBD, unbBD)
+ case (false, true, false, _) => (r.minLengthValue, unbBD)
+ case (true, _, _, _) =>
+ SDE("Facet length cannot be defined with minLength and maxLength
facets")
Review Comment:
This is a minor organizational thing, but it might be helpful to move this
case right after the `(true, false,false, _)`case. That way the first two cases
are when `hasLength` is true--we either use it or error if either
minLength/maxLength is defined. That handles all the cases where hasLength is
true, so it's maybe a little more mentally easier to think about the more
complex minLength/maxLength cases that follow, since we don't have to consider
hasLength anymore.
Also, should this be an assert.invariantFailed, since xerces should fail
before we get here if length is defined with a minLength/maxLength?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
