[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for
textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r369625873
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala
##
@@ -605,7 +605,12 @@ trait ElementBaseGrammarMixin
}
private lazy val textStandardNumber = prod("textStandardNumber",
textNumberRep == TextNumberRep.Standard) {
-ConvertTextCombinator(this, stringValue, textConverter)
+val converter = textStandardBaseDefaulted match {
+ case 10 => textConverter
+ case 2 | 8 | 16 => textStandardNonBaseTenConverter
Review comment:
Ah, I thought you were just talking about base e. I'll merge 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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for
textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r369600719
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala
##
@@ -605,7 +605,12 @@ trait ElementBaseGrammarMixin
}
private lazy val textStandardNumber = prod("textStandardNumber",
textNumberRep == TextNumberRep.Standard) {
-ConvertTextCombinator(this, stringValue, textConverter)
+val converter = textStandardBaseDefaulted match {
+ case 10 => textConverter
+ case 2 | 8 | 16 => textStandardNonBaseTenConverter
Review comment:
@mbeckerle , thoughts on base 256? I think all other comments have been
addressed and this commit is ready to merge otherwise.
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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for
textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366414420
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala
##
@@ -605,7 +605,12 @@ trait ElementBaseGrammarMixin
}
private lazy val textStandardNumber = prod("textStandardNumber",
textNumberRep == TextNumberRep.Standard) {
-ConvertTextCombinator(this, stringValue, textConverter)
+val converter = textStandardBaseDefaulted match {
+ case 10 => textConverter
+ case 2 | 8 | 16 => textStandardNonBaseTenConverter
Review comment:
What's the use case for base-256? A base-256 encoded number would probably
be full of non-display characters, so I'm not sure why that would ever be used
in practice?
Implementation also becomes a bit more difficult, since right now we just
rely on Java utils to converts to/from base-2,8,16 encoded numbers. Supporting
anything means we have to implement our own logic since that's all java
supports. Not hard, but more work and more code to maintain. Just want to make
sure there's a good use case for it.
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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for
textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366414689
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala
##
@@ -625,6 +630,15 @@ trait ElementBaseGrammarMixin
}
}
+ private lazy val textStandardNonBaseTenConverter = {
+primType match {
+ case PrimType.Double | PrimType.Float | PrimType.Decimal =>
Review comment:
Will do, I'll also fix up Zoned with a similar logic while I'm there.
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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for
textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366414758
##
File path:
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/NonBaseTenTextNumberParser.scala
##
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.processors.parsers
+
+import java.math.{ BigInteger => JBigInt }
+import java.lang.{ Number => JNumber }
+
+import org.apache.daffodil.dpath.NodeInfo
+import org.apache.daffodil.processors.ElementRuntimeData
+
+class ConvertNonBaseTenTextNumberParser(
+ override val context: ElementRuntimeData,
+ base: Int)
+ extends TextPrimParser {
+
+ override lazy val runtimeDependencies = Vector()
+
+ private val primNumeric =
context.optPrimType.get.asInstanceOf[NodeInfo.PrimType.PrimNumeric]
+
+ final def parse(state: PState): Unit = {
+
+val node = state.simpleElement
+val baseStr = node.dataValueAsString
+
+if (baseStr == "") {
+ PE(state, "Unable to parse %s from empty string",
context.optPrimType.get.globalQName)
+ return
+}
+
+// must explicitly check for and error on text that start with + or -
+// because DFDL does not allow a leading sign character, but parseInt and
+// friends will accept them
+val firstChar = baseStr(0)
+if (firstChar == '-' || firstChar == '+') {
Review comment:
WIll do
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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for textStandardBase URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366317636 ## File path: daffodil-test-ibm1/src/test/resources/test-suite/ibm-contributed/dpaext2.tdml ## @@ -145,7 +145,7 @@ +description="Section 13.5 Specification of number base - 2,8, and 16" roundTrip="twoPass"> Review comment: I'll also add that a few of other tests in this file use roundTrip="twoPass", so it is at least consistent with those. 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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for textStandardBase URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366315437 ## File path: daffodil-test-ibm1/src/test/resources/test-suite/tresys-contributed/BG.tdml ## @@ -25,25 +25,25 @@ description="Text number properties"> Review comment: I don't think these are actually IBM tests. They're just incorrectly in the test-ibm1 project (looks like they got moved in commit 406c470a5b in 2013). All the AA-BG tests were either added by Tresys or inherited from the original Daffodil codebase. So it's fine to change these (and they were definitely broken). 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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for textStandardBase URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366310199 ## File path: daffodil-test-ibm1/src/test/resources/test-suite/ibm-contributed/dpaext2.tdml ## @@ -145,7 +145,7 @@ +description="Section 13.5 Specification of number base - 2,8, and 16" roundTrip="twoPass"> Review comment: I usually prefer two-pass over none since they both require that the parse matches the infoset, but you also get some unparse testing to at least make sure things unparse somewhat correctly. Which is all I'm really caring about in this particular test. I generally avoid changing IBM tests (unless they're completely broken), so this gives me some assurance that this is doing the right thing. (Note that two pass is required because there's a leading zero, which doesn't unparse). The new tests added for textStandardBase are more strict, and do have separate tests for parse vs unparse when we want to ensure that unparse is doing the right thing. So while this test could potentially hide issues due to two pass, other tests should catch those issues. 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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for
textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366306859
##
File path:
daffodil-lib/src/main/scala/org/apache/daffodil/schema/annotation/props/ByHandMixins.scala
##
@@ -95,10 +95,34 @@ object TextNumberBase {
case "8" => 8
case "10" => 10
case "16" => 16
- case _ => self.schemaDefinitionError("Illegal number base: " + str) //
validation will have checked. So this shoudn't happen.
+ case _ => self.schemaDefinitionError("For property textStandardBase,
value must be 2, 8, 10, or 16. Found: %s", str)
Review comment:
Yep, the SDE in ElemenetBaseGrammarMixin should just be an assert.
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] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase
stevedlawrence commented on a change in pull request #314: Add support for
textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366305787
##
File path:
daffodil-core/src/main/scala/org/apache/daffodil/grammar/ElementBaseGrammarMixin.scala
##
@@ -625,6 +630,16 @@ trait ElementBaseGrammarMixin
}
}
+ private lazy val textStandardNonBaseTenConverter = {
+primType match {
+ case PrimType.Double | PrimType.Float | PrimType.Decimal =>
+SDE("dfdl:textStandardBase=\"%s\" cannot be used with %s",
textStandardBaseDefaulted, primType.globalQName)
+ case _: NodeInfo.Numeric.Kind => ConvertNonBaseTenTextNumberPrim(this)
+ case PrimType.HexBinary | PrimType.Boolean | PrimType.Date |
PrimType.Time | PrimType.DateTime | PrimType.AnyURI | PrimType.String =>
Review comment:
Yeah, that's probably the right thing to do in this case. I usually like to
list everything because scala will warn you if the match/case isn't exhaustive.
So it sort of forces you to think about every case. And if we add a new type
(e.g. like we did recently for blobs) this will error and we'll be forced to
think about how this new type applies to this match. But in this case I'm not
sure if it's worth it.
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
