stevedlawrence commented on code in PR #1569:
URL: https://github.com/apache/daffodil/pull/1569#discussion_r2406185613
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -126,17 +126,35 @@ trait SeparatedSequenceChildParseResultHelper extends
SequenceChildParseResultHe
): Unit = {
if ((sscb eq PositionalTrailingStrict)) {
- val resultToTest = priorResultOfTry
- resultToTest match {
- case ParseAttemptStatus.AbsentRep | ParseAttemptStatus.EmptyRep =>
- parser.PE(
- pstate,
- "Empty trailing optional elements are not allowed when
dfdl:separatorSuppressionPolicy='trailingEmptyStrict'"
- )
- case _ => // ok
+ if (checkTrailingOptionalElements(resultOfTry)) {
+ ParseErrorEmptyTrailingStrict(parser, pstate)
+ } else if (checkTrailingOptionalElements(priorResultOfTry)) {
+ ParseErrorEmptyTrailingStrict(parser, pstate)
Review Comment:
The blocks of these two if statements are the same, I would suggest just
doing something like:
```scala
if (checkTrailingOptionalElements(resultOfTry) ||
checkTrailingOptionalElements(priorResultOfTry) {
parser.PE(...)
}
```
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -126,17 +126,35 @@ trait SeparatedSequenceChildParseResultHelper extends
SequenceChildParseResultHe
): Unit = {
if ((sscb eq PositionalTrailingStrict)) {
- val resultToTest = priorResultOfTry
- resultToTest match {
- case ParseAttemptStatus.AbsentRep | ParseAttemptStatus.EmptyRep =>
- parser.PE(
- pstate,
- "Empty trailing optional elements are not allowed when
dfdl:separatorSuppressionPolicy='trailingEmptyStrict'"
- )
- case _ => // ok
+ if (checkTrailingOptionalElements(resultOfTry)) {
+ ParseErrorEmptyTrailingStrict(parser, pstate)
+ } else if (checkTrailingOptionalElements(priorResultOfTry)) {
+ ParseErrorEmptyTrailingStrict(parser, pstate)
+ } else {
+ // ok
}
}
}
+
+ private def ParseErrorEmptyTrailingStrict(
Review Comment:
I'm not sure this should be capitalized, it make its usage look like it's
calling an object apply method.
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -126,17 +126,35 @@ trait SeparatedSequenceChildParseResultHelper extends
SequenceChildParseResultHe
): Unit = {
if ((sscb eq PositionalTrailingStrict)) {
- val resultToTest = priorResultOfTry
- resultToTest match {
- case ParseAttemptStatus.AbsentRep | ParseAttemptStatus.EmptyRep =>
- parser.PE(
- pstate,
- "Empty trailing optional elements are not allowed when
dfdl:separatorSuppressionPolicy='trailingEmptyStrict'"
- )
- case _ => // ok
+ if (checkTrailingOptionalElements(resultOfTry)) {
+ ParseErrorEmptyTrailingStrict(parser, pstate)
+ } else if (checkTrailingOptionalElements(priorResultOfTry)) {
+ ParseErrorEmptyTrailingStrict(parser, pstate)
+ } else {
+ // ok
}
}
}
+
+ private def ParseErrorEmptyTrailingStrict(
+ parser: SequenceChildParser,
+ pstate: PState
+ ): Unit = {
+ parser.PE(
+ pstate,
+ "Empty trailing optional elements are not allowed when
dfdl:separatorSuppressionPolicy='trailingEmptyStrict'"
+ )
+ }
+
+ private def checkTrailingOptionalElements(
+ resultToTest: ParseAttemptStatus
+ ): Boolean = {
+ Seq(
+ ParseAttemptStatus.MissingItem,
+ ParseAttemptStatus.AbsentRep,
+ ParseAttemptStatus.EmptyRep
+ ).contains(resultToTest)
+ }
Review Comment:
Having a comment here would be helpful to. MIssing and Absent are both types
of Failure parse status, so I would think that indicates error. But I guess
because these are optional trailing elements those are not considered errors?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/parsers/SeparatedSequenceChildParseResultHelper.scala:
##########
@@ -126,17 +126,35 @@ trait SeparatedSequenceChildParseResultHelper extends
SequenceChildParseResultHe
): Unit = {
if ((sscb eq PositionalTrailingStrict)) {
- val resultToTest = priorResultOfTry
- resultToTest match {
- case ParseAttemptStatus.AbsentRep | ParseAttemptStatus.EmptyRep =>
- parser.PE(
- pstate,
- "Empty trailing optional elements are not allowed when
dfdl:separatorSuppressionPolicy='trailingEmptyStrict'"
- )
- case _ => // ok
+ if (checkTrailingOptionalElements(resultOfTry)) {
Review Comment:
Yeah, I think a comment explaining this is really important.
For example, why do we need to check both resultOfTry *AND*
priorResultOfTry? My intuition is only the most recently parsed thing should
matter for trailing empty since we only care about the trailing thing.
Also, why is this only checked for trailingEmptyStrict? Why do we not need
to check for other thingslike PositionTrailingLax?
--
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]
