mbeckerle commented on code in PR #1289:
URL: https://github.com/apache/daffodil/pull/1289#discussion_r1747608330
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/RestrictionUnion.scala:
##########
@@ -102,10 +107,10 @@ final class Restriction private (xmlArg: Node, val
simpleTypeDef: SimpleTypeDefB
* Exclusive - restriction either has a baseType or a direct primType.
*/
lazy val (optDirectPrimType, optBaseTypeDef: Option[GlobalSimpleTypeDef]) = {
- val optPT = schemaSet.getPrimitiveType(baseQName)
+ val optPT = PrimType.fromQName(baseQName)
Review Comment:
Looks incorrect.
PrimType.fromQName expects the baseQName to be a primitive type name.
The schemaSet.getPrimitiveType(baseQName) should take the baseQName and run
up it's definition chain until it finds the eventual primitive type it is
derived from. That's why it's a method on schemaSet, so that reference chains
can be followed to other global type defs.
Is this code really trying to both chase the chain back to the primitive
base type while also assuring that the referenced types all exist? I think
reference integrity really should be checked elsewhere, not here (and likely
already is), and this can just call schemaSet.getPrimitiveType(baseQName) and
error if none comes back.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/primitives/PrimitivesExpressions.scala:
##########
@@ -197,6 +202,7 @@ case class SetVariable(stmt: DFDLSetVariable, override val
term: Term)
override lazy val exprText = stmt.value
override lazy val exprNamespaces = stmt.xml.scope
+ override lazy val exprTargetNamespace = stmt.annotatedSC.targetNamespace
Review Comment:
This is pretty paindful that everywhere we have xyzNamespaces to get the
scopes we have in parallel xyzTargetNamespace also.
These should be one context object, not two both of which must be maintained
in parallel in all these different places.
I.e., one tuple of these two things, not two separate things passed all the
time.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala:
##########
@@ -413,87 +411,52 @@ final class SchemaSet private (
* These all return factories for the objects, not the objects themselves.
*/
def getGlobalElementDecl(refQName: RefQName) = {
- val s = getSchema(refQName.namespace)
- val res = s.flatMap { s =>
- {
- val ged = s.getGlobalElementDecl(refQName.local)
- ged
- }
+ getSchema(refQName.namespace).flatMap {
+ _.getGlobalElementDecl(refQName.local)
}
- res
}
- def getGlobalSimpleTypeDef(refQName: RefQName) =
getSchema(refQName.namespace).flatMap {
- _.getGlobalSimpleTypeDef(refQName.local)
- }
-
- def getGlobalSimpleTypeDefNoPrim(
- refQName: RefQName,
- prop: String,
- context: ThrowsSDE
- ): GlobalSimpleTypeDef = {
- val gstd = getGlobalSimpleTypeDef(refQName)
- gstd.getOrElse {
- val isPrimitive = getPrimitiveType(refQName).isDefined
- val msg =
- if (isPrimitive) s"The $prop property cannnot resolve to a primitive
type: $refQName"
- else s"Failed to resolve $prop to a global simpleType definition:
$refQName"
- context.schemaDefinitionError(msg)
+ def getGlobalSimpleTypeDef(refQName: RefQName) = {
Review Comment:
Do all these getters return option types?
You made the Option type explicit for the elements above, so I think for the
other global things these should also have return types declared. I guess it is
considered good style to provide return types on all public and protected
methods anyway.
--
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]
