stevedlawrence commented on code in PR #1434:
URL: https://github.com/apache/daffodil/pull/1434#discussion_r1969690696
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dpath/DFDLExpressionParser.scala:
##########
@@ -181,22 +181,35 @@ class DFDLPathExpressionParser[T <: AnyRef](
expressionAST
}
case NoSuccess(msg, next) => {
Review Comment:
The scala parser combinator documentation about
[NoSuccess](https://javadoc.io/static/org.scala-lang.modules/scala-parser-combinators_2.13/2.4.0/scala/util/parsing/combinator/Parsers$NoSuccess$.html)
says this:
> An extractor so case NoSuccess(msg, next) can be used in matches.
>
> Note: On Scala 2.13, using this extractor leads to an exhaustivity warning:
>
> def m(r: ParseResult[Int]) = r match {
> case Success(i) => ...
> case NoSuccess(msg, _) => ... // "warning: match may not be exhaustive"
>
> To eliminate this warning, use the irrefutable NoSuccess.I extractor. Due
to binary compatibility, NoSuccess itself cannot be changed.
And below that `NoSuccess.I` scaladoc has this example:
```scala
case NoSuccess.I(msg, next)
```
I think switching to that would avoid the need for the Error and Failure
cases?
##########
daffodil-core/src/test/scala/org/apache/daffodil/core/grammar/primitives/TestPrimitives.scala:
##########
@@ -62,31 +64,36 @@ class TestPrimitives {
val optMatch = re.findFirstMatchIn("'P'###012V34'P';N#N")
println(optMatch)
optMatch match {
- case Some(re("'P'", "###012", "34", "'P'", "N", "#", "N")) => // ok
+ case Some(m) if m.subgroups == List("'P'", "###012", "34", "'P'", "N",
"#", "N") => // ok
+ case _ => fail()
}
}
@Test def testVRegexOnlyPositivePattern(): Unit = {
val re = TextNumberPatternUtils.vRegexStandard
val Some(myMatch) = re.findFirstMatchIn("A###012V34B")
- myMatch match {
- case re("A", "###012", "34", "B", null, null, null) =>
+ myMatch.subgroups match {
+ case List("A", "###012", "34", "B", null, null, null) => // ok
+ case _ => fail()
}
Review Comment:
Why did this test and other tests have to change when others at the top of
this file look exactly the same but didn't need changes? Does it have to do
with the null's or somthing?
I would also suggest making yoru changes consistent. Sometimes you do
`Some(m) if m.subgroups`, sometimes you do `val Some(myMatch) = ...` with `case
myMatch.subgroups`. If we have to change these tests we should try to do so in
a consistent way.
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/grammar/ElementBaseGrammarMixin.scala:
##########
@@ -1392,6 +1392,7 @@ trait ElementBaseGrammarMixin
notYetImplemented("lengthKind='endOfParent' for complex type")
case LengthKind.EndOfParent =>
notYetImplemented("lengthKind='endOfParent' for simple type")
+ case _ => Assert.impossibleCase("default case should not be reached")
Review Comment:
If it's only a couple of cases, can we explicitly add them as impossible
cases? This match case is complicated enough that it might be helpful to self
document exactly what cases aren't supported here.
--
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]
