stevedlawrence commented on code in PR #1579:
URL: https://github.com/apache/daffodil/pull/1579#discussion_r2499060165
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Term.scala:
##########
@@ -511,6 +511,24 @@ trait Term
* Note that this currently only requires OVC and Optionality since defaults
* aren't fully implemented everywhere. This function may need to change when
* defaults are fully implemented.
+ *
+ * Hidden IVCs — elements computed during parse but never represented in the
+ * infoset or needed during unparse — are not meaningful for unparsing and
+ * should not be supported. So we remove !e.isRepresented from the
ElementBase
+ * case.
+ *
+ * Unparsing should instead require IVC elements (minOccurs = maxOccurs = 1)
+ * to appear in the infoset, even though their values are ignored at unparse
+ * time. This behavior is more intuitive, and is in line with IVC being
MustExist in
+ * canUnparseIfNoEvents and preserves expected branch selection semantics for
+ * choice/dispatch constructs (e.g., the “raster” schema case).
Review Comment:
Suggest we remove mention of "raster", this is not a publicly available
schema so doesn't really provide value.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/Term.scala:
##########
@@ -511,6 +511,24 @@ trait Term
* Note that this currently only requires OVC and Optionality since defaults
* aren't fully implemented everywhere. This function may need to change when
* defaults are fully implemented.
+ *
+ * Hidden IVCs — elements computed during parse but never represented in the
+ * infoset or needed during unparse — are not meaningful for unparsing and
+ * should not be supported. So we remove !e.isRepresented from the
ElementBase
Review Comment:
If someone was reading this code/comment for the first time, I think saying
"So we remove !e.isRepresented" is potentially confusing. They don't know that
the code used to consider e.isRepresented, so it feels like it would be
confusing why you even bring it up.
In fact, as I read through this whole comment a couple times, it feels more
appropriate for a commit message. It isn't so much explaining what the code
does, but is really adding justification for why checking e.isRepresented was
not the correct thing do to. Which is exactly the kind of thing that should be
in a commit message rather than a comment.
I think if we want to call out IVC here, which we should probably do so it's
clear we are intentionally not allowing it to be unparsed if hidden, we can
just say something like:
> Note that even though inputValueCalc elements are essentially ignored for
unparsing, they are still considered required elements
(canBeAbsnsetFromUnparseInfoset returns false) and so cannot be unparsed if
they do not appear in the infoset (e.g. inside a hidden grep ref). This does
not necessarily exclude inputValueCalc elements from appearing in hidden
groups, but they must be in the non-default branch of a choice, or a child to
an optional complex type or non-required array.
I'm not sure that's the best comment, but something along those lines that
explains the current state of the code.
--
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]
