[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392523492
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -217,27 +217,37 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
}
override lazy val unparser: Unparser = {
-val optDefaultableBranchUnparser = if (ch.isHidden) {
- // this call is necessary since it will throw an SDE if no choice branch
- // was defaultable
- ch.childrenThatAreRecursivelyNotDefaultableOrOVC
+/*
+ * Since it's impossible to know the hiddenness for terms at this level
(unless
+ * they're a hiddenGroupRef), we always attempt to find a defaultable
unparser.
+ * If we find one, and at runtime, we're in a hidden state, we can use
that. If we
+ * are not in a hidden state, it doesn't get used. If we don't find one,
and we encounter
+ * a hidden group ref here, we know we'll run into that issue at runtime,
so we SDE.
+ */
+val optDefaultableBranchUnparser = {
+ val defaultableBranches = ch.groupMembers.find {
_.isFullyDefaultableOrOVC }
+ if (defaultableBranches.nonEmpty) {
+val defaultableBranchRD = defaultableBranches.get.runtimeData
- val defaultableBranches = ch.groupMembers.filter {
_.childrenThatAreRecursivelyNotDefaultableOrOVC.length == 0 }
- Assert.invariant(defaultableBranches.length > 0)
- if (ch.isHidden && defaultableBranches.length > 1) {
-SDW(WarnID.ChoiceInsideHiddenGroup, "xs:choice inside a hidden group
has unparse ambiguity: multiple branches exist with all children either
defaulable or have the dfdl:outputValueCalc property set. The first branch will
be chosen during unparse. Defaultable branches are:\n%s",
- defaultableBranches.mkString("\n"))
+// Choices inside a hidden group ref are slightly different because we
+// will never see events for any of the branches. Instead, we will just
+// always pick the branch in which every thing is defaultble or OVC.
+val defaultableBranchUnparser =
alternatives.find(_.context.runtimeData =:= defaultableBranchRD).get.unparser
+Some(defaultableBranchUnparser)
+ } else {
+ch match {
+ case cgr: GroupRef if cgr.isHidden => {
+/*
+ * This is a requirement for terms inside a hidden group. If we're
a hidden group ref, we know
+ * our descendants need this requirements, so it's a SDE if they
don't meet it.
Review comment:
"need this requirements"... ambiguous. Did you mean "meets this requirement"
or "requires this" ?
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392525136
##
File path:
daffodil-cli/src/it/scala/org/apache/daffodil/debugger/TestCLIDebugger.scala
##
@@ -900,10 +900,14 @@ class TestCLIdebugger {
}
@Test def test_CLI_Debugger_InfoHidden_1() {
-val schemaFile =
Util.daffodilPath("daffodil-test/src/test/resources/org/apache/daffodil/section14/sequence_groups/SequencesWithHiddenRefs.dfdl.xsd")
-val inputFile =
Util.daffodilPath("daffodil-cli/src/it/resources/org/apache/daffodil/CLI/input/test_isHidden1.txt")
-val (testSchemaFile, testInputFile) = if (Util.isWindows)
(Util.cmdConvert(schemaFile), Util.cmdConvert(inputFile)) else (schemaFile,
inputFile)
-
+val schemaFile = Util.daffodilPath(
Review comment:
Really bugs me that every one of these tests (not just your new ones) has
this repeated isWindows conditionalization.
We really should be able to abstract this away so that tests don't have this
baggage over and over.
These same 6 lines of code are repeated a lot.
```
val (testSchemaFile, testInputFile) = if (Util.isWindows) {
(Util.cmdConvert(schemaFile),
Util.cmdConvert(inputFile.getAbsolutePath))
} else {
(schemaFile, inputFile)
}
val shell = if (Util.isWindows) Util.start("", envp =
DAFFODIL_JAVA_OPTS) else Util.start("")
```
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #332: Refactor isHidden
mbeckerle commented on a change in pull request #332: Refactor isHidden
URL: https://github.com/apache/incubator-daffodil/pull/332#discussion_r392523694
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/ChoiceCombinator.scala
##
@@ -257,9 +267,8 @@ case class ChoiceCombinator(ch: ChoiceTermBase,
alternatives: Seq[Gram])
if (mapValues.isEmpty)
new NadaUnparser(null)
else {
- val firstBranchUnparser = alternatives(0).unparser
new ChoiceCombinatorUnparser(ch.modelGroupRuntimeData, eventUnparserMap,
-choiceLengthInBits, optDefaultableBranchUnparser, firstBranchUnparser)
+choiceLengthInBits, optDefaultableBranchUnparser)
Review comment:
Before committing this, we really must test mil-std-2045 and vmf. These have
been turning up many problems related to choices for me.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-daffodil-site] olabusayoT opened a new pull request #19: WIP: Daffodil 2281 ishidden
olabusayoT opened a new pull request #19: WIP: Daffodil 2281 ishidden URL: https://github.com/apache/incubator-daffodil-site/pull/19 Updates from implementation. Set to WIP as main PR is not yet merged/approved 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
