mbeckerle commented on code in PR #911:
URL: https://github.com/apache/daffodil/pull/911#discussion_r1068411624
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/PrimitivesTextNumber.scala:
##########
@@ -275,17 +410,67 @@ case class ConvertTextStandardNumberPrim(e: ElementBase)
extends Terminal(e, true)
with ConvertTextNumberMixin {
+
final override protected lazy val textDecimalVirtualPointFromPattern: Int = {
- val r = TextNumberPatternUtils.vregexStandard
- r.findFirstMatchIn(patternStripped) match {
- case Some(r(_, _, afterV, _, _, _, _, _)) => afterV.length
- case None =>
- e.SDE(
- s"""The dfdl:textNumberPattern '%s' contains 'V' (virtual decimal
point).
- | Other than the sign indicators, it can contain only
- | '#', then digits 0-9 then 'V' then digits 0-9.
- | The positive part of the dfdl:textNumberPattern is
mandatory.""".stripMargin('|'),
+ if (hasV) {
+ val r = TextNumberPatternUtils.vRegexStandard
+ r.findFirstMatchIn(patternNoQuoted) match {
+ case Some(r(_, _, afterV, _, _, _, _, _)) => afterV.length
+ case None =>
+ e.SDE(
+ s"""The dfdl:textNumberPattern '%s' contains 'V' (virtual decimal
point).
+ | Other than the sign indicators, it can contain only
+ | '#', then digits 0-9 then 'V' then digits 0-9.
+ | The positive part of the dfdl:textNumberPattern is
mandatory.""".stripMargin('|'),
+ pattern)
+ }
+ } else if (hasP) {
+ val rr = TextNumberPatternUtils.pOnRightRegexStandard
+ val rl = TextNumberPatternUtils.pOnLeftRegexStandard
+ val rightMatch = rr.findFirstMatchIn(patternNoQuoted)
+ val leftMatch = rl.findFirstMatchIn(patternNoQuoted)
+ (leftMatch, rightMatch) match {
+ case (None, None) => e.SDE(
+ """The dfdl:textNumberPattern '%s' contains 'P' (virtual decimal point
positioners).
+ |However, it did not match the allowed syntax which allows the
sign indicator
+ |plus digits on only one side of the P symbols.""".stripMargin,
pattern)
+ case (Some(rl(_, ps, digits, _, negPre, negPs, negDigits, _)), None)
=> {
+ checkPPPMatchSyntax(ps, digits, negPre, negPs, negDigits)
+ ps.length + digits.length
+ }
+ case (None, Some(rr(_, digits, ps, _, negPre, negDigits, negPs, _)))
=> {
+ checkPPPMatchSyntax(ps, digits, negPre, negPs, negDigits)
+ - ps.length // negate value.
+ }
+ case _ => Assert.invariantFailed("Should not match both left P and
right P regular expressions.")
+ }
+ } else {
+ 0 // there is no V nor P, so there is no virtual decimal point scaling
involved.
+ }
+ }
+
+ /**
+ * Check for common errors in the pattern matches for 'P' patterns.
+ *
+ * @param ps match string for positive pattern P characters
+ * @param digits match string for positive pattern digits (and sharps)
+ * @param negPre negative prefix match (null if there is no negative
pattern at all)
+ * @param negPs match string for negative pattern P character (or null)
+ * @param negDigits match string for negative pattern P digits (and sharps)
+ */
+ private def checkPPPMatchSyntax(ps: String, digits: String, negPre: String,
negPs: String, negDigits: String): Unit = {
+ Assert.invariant(ps.length >= 1)
+ digits.length >= 1
Review Comment:
Yes, I think was intended to be an assertion.
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/PrimitivesTextNumber.scala:
##########
@@ -80,14 +99,72 @@ object TextNumberPatternUtils {
// don't forget the ^ and $ (start of data, end of data) because we want
// the match to consume all the characters, starting at the beginning.
val vPattern=
-
s"""^${posPrefix}${posBeforeV}V${posAfterV}${posSuffix}(?:;${negPrefix}${negBeforeV}V${negAfterV}${negSuffix})?$$"""
+ s"""^${posPrefix}${posBeforeV}V${posAfterV}${posSuffix}""" ++
+ s"""(?:;${negPrefix}${negBeforeV}(?:V${negAfterV}${negSuffix})?)?$$"""
Review Comment:
Good catch.
Actually, while normally we think the positive and negative patterns should
match, minus the sign differences, turns out, the number in the middle part of
the negative pattern is supposed to be ignored.
The example usally given is this "###0.00;(#)" which means the same as
"###0.00;(###0.00)".
I will fix this "optionality" aspect of the middle number part of the
negative pattern.
I think my regex requires the negative pattern to have a V in it. (When it's
a V pattern), which should not be required in the negative pattern since it's
going to be ignored anyway.
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/PrimitivesTextNumber.scala:
##########
@@ -275,17 +410,67 @@ case class ConvertTextStandardNumberPrim(e: ElementBase)
extends Terminal(e, true)
with ConvertTextNumberMixin {
+
final override protected lazy val textDecimalVirtualPointFromPattern: Int = {
- val r = TextNumberPatternUtils.vregexStandard
- r.findFirstMatchIn(patternStripped) match {
- case Some(r(_, _, afterV, _, _, _, _, _)) => afterV.length
- case None =>
- e.SDE(
- s"""The dfdl:textNumberPattern '%s' contains 'V' (virtual decimal
point).
- | Other than the sign indicators, it can contain only
- | '#', then digits 0-9 then 'V' then digits 0-9.
- | The positive part of the dfdl:textNumberPattern is
mandatory.""".stripMargin('|'),
+ if (hasV) {
+ val r = TextNumberPatternUtils.vRegexStandard
+ r.findFirstMatchIn(patternNoQuoted) match {
+ case Some(r(_, _, afterV, _, _, _, _, _)) => afterV.length
+ case None =>
+ e.SDE(
+ s"""The dfdl:textNumberPattern '%s' contains 'V' (virtual decimal
point).
+ | Other than the sign indicators, it can contain only
+ | '#', then digits 0-9 then 'V' then digits 0-9.
+ | The positive part of the dfdl:textNumberPattern is
mandatory.""".stripMargin('|'),
+ pattern)
+ }
+ } else if (hasP) {
+ val rr = TextNumberPatternUtils.pOnRightRegexStandard
+ val rl = TextNumberPatternUtils.pOnLeftRegexStandard
+ val rightMatch = rr.findFirstMatchIn(patternNoQuoted)
+ val leftMatch = rl.findFirstMatchIn(patternNoQuoted)
+ (leftMatch, rightMatch) match {
+ case (None, None) => e.SDE(
+ """The dfdl:textNumberPattern '%s' contains 'P' (virtual decimal point
positioners).
+ |However, it did not match the allowed syntax which allows the
sign indicator
+ |plus digits on only one side of the P symbols.""".stripMargin,
pattern)
+ case (Some(rl(_, ps, digits, _, negPre, negPs, negDigits, _)), None)
=> {
+ checkPPPMatchSyntax(ps, digits, negPre, negPs, negDigits)
+ ps.length + digits.length
+ }
+ case (None, Some(rr(_, digits, ps, _, negPre, negDigits, negPs, _)))
=> {
+ checkPPPMatchSyntax(ps, digits, negPre, negPs, negDigits)
+ - ps.length // negate value.
+ }
+ case _ => Assert.invariantFailed("Should not match both left P and
right P regular expressions.")
+ }
+ } else {
+ 0 // there is no V nor P, so there is no virtual decimal point scaling
involved.
+ }
+ }
+
+ /**
+ * Check for common errors in the pattern matches for 'P' patterns.
+ *
+ * @param ps match string for positive pattern P characters
+ * @param digits match string for positive pattern digits (and sharps)
+ * @param negPre negative prefix match (null if there is no negative
pattern at all)
+ * @param negPs match string for negative pattern P character (or null)
+ * @param negDigits match string for negative pattern P digits (and sharps)
+ */
+ private def checkPPPMatchSyntax(ps: String, digits: String, negPre: String,
negPs: String, negDigits: String): Unit = {
+ Assert.invariant(ps.length >= 1)
+ digits.length >= 1
+ if (negPre ne null) {
+ e.schemaDefinitionUnless(ps == negPs,
+ """In the dfdl:textNumberPattern '%s' the P Symbols do not match
+ | between positive and negative patterns""".stripMargin,
+ pattern)
+ e.schemaDefinitionUnless(digits == negDigits,
+ """In the dfdl:textNumberPattern '%s' the '#' and digit symbols do not
match
+ | between positive and negative patterns""".stripMargin,
+ pattern)
}
Review Comment:
Discussed above. I will try to allow the negative pattern to be ignored.
But I think yes, if the negative number part of the pattern is other than a
stub like just "#" or "0" we should warn that it will be ignored unless it is
identical to the number part of the positive pattern.
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/PrimitivesTextNumber.scala:
##########
@@ -170,13 +279,23 @@ trait ConvertTextNumberMixin {
final protected def pattern = e.textNumberPattern
/**
- * Checks a pattern for suitability with V (virtual decimal point) in the
pattern
- * in the context of the corresponding textNumberRep. Computes number of
digits to right of the V.
+ * Analogous to the property dfdl:binaryDecimalVirtualPoint
+ *
+ * Value is 0 if there is no virtual decimal point.
+ *
+ * Value is the number of digits to the right of the 'V' for V patterns.
*
- * SDE if pattern has a syntax error.
+ * For P patterns, the value is the number of P characters when
+ * the P chars are on the right of the digits, or it is negated if the
+ * P chars are on the left of the digits.
*
- * @return the number of digits to the right of the V character. Must be 1
or greater.
+ * Examples:
*
+ * "000.00" returns 2, so text "12345" yields 123.45
Review Comment:
Yes, V
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/PrimitivesTextNumber.scala:
##########
@@ -275,17 +410,67 @@ case class ConvertTextStandardNumberPrim(e: ElementBase)
extends Terminal(e, true)
with ConvertTextNumberMixin {
+
final override protected lazy val textDecimalVirtualPointFromPattern: Int = {
- val r = TextNumberPatternUtils.vregexStandard
- r.findFirstMatchIn(patternStripped) match {
- case Some(r(_, _, afterV, _, _, _, _, _)) => afterV.length
- case None =>
- e.SDE(
- s"""The dfdl:textNumberPattern '%s' contains 'V' (virtual decimal
point).
- | Other than the sign indicators, it can contain only
- | '#', then digits 0-9 then 'V' then digits 0-9.
- | The positive part of the dfdl:textNumberPattern is
mandatory.""".stripMargin('|'),
+ if (hasV) {
+ val r = TextNumberPatternUtils.vRegexStandard
+ r.findFirstMatchIn(patternNoQuoted) match {
+ case Some(r(_, _, afterV, _, _, _, _, _)) => afterV.length
+ case None =>
+ e.SDE(
+ s"""The dfdl:textNumberPattern '%s' contains 'V' (virtual decimal
point).
+ | Other than the sign indicators, it can contain only
+ | '#', then digits 0-9 then 'V' then digits 0-9.
+ | The positive part of the dfdl:textNumberPattern is
mandatory.""".stripMargin('|'),
+ pattern)
+ }
Review Comment:
That's correct. V is checked first, and if V is found, other things
incompatible with V (like P) raise this diagnostic.
##########
daffodil-core/src/main/scala/org/apache/daffodil/grammar/primitives/PrimitivesTextNumber.scala:
##########
@@ -114,21 +191,66 @@ object TextNumberPatternUtils {
* Checks a pattern for suitability with V (virtual decimal point) in the
pattern
* in the context of zoned textNumberRep.
*
- * This is broken out separately for unit testing purposes.
+ * This method is here in this object for unit testing purposes.
*
* @param patternStripped the dfdl:textNumberPattern pattern - with all
quoting removed.
* @return None if the pattern is illegal syntax for use with V (virtual
decimal point)
* otherwise Some(N) where N is the number of digits to the right of
the V character.
*/
- private[primitives] def textDecimalVirtualPointForZoned(patternStripped:
String): Option[Int] = {
- val r = TextNumberPatternUtils.vregexZoned
+ def textNumber_V_DecimalVirtualPointForZoned(patternStripped: String):
Option[Int] = {
Review Comment:
I just removed the package private because the whole object is package
private, which allows this to be unit tested.
Really, hoisting the things that have subtle regex behavior so they're
easily unit tested is pretty important in getting this code correct, so I'm not
inclined to change this. I tend to want to do the opposite and hoist more
things over here and add more unit tests.
--
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]
