You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/01/13 14:17:28 UTC

[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #314: Add support for textStandardBase

stevedlawrence opened a new pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314
 
 
   Add new primitive, parser, and unparser to handle non base-10 text
   numbers. Actual logic from converting to/from bases is handled by Java's
   builtin functions.
   
   To maintain backwards compatibility, if the textStandardBase property is
   not defined it defaults to "10" unless the requireTextStandardBaseProperty
   tunable is set to true, default to false.
   
   Also noticed some unordered sequence tests were commented out and work
   with minor tweaks due to incorrect tests.
   
   DAFFODIL-840

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366369679
 
 

 ##########
 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:
   Please add to comment that DFDL spec says textNumberPattern is not used when textStandardBase is not 10. 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] bsloane1650 commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366114418
 
 

 ##########
 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:
   Can this be a wildcard match instead of listing every type?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence merged pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314
 
 
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] bsloane1650 commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366413287
 
 

 ##########
 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:
   The DFDL spec is clear on what valid values are for textStandardBase, and they are only 2, 8, 10, 16.  I would want a compelling reason to deviate from this.
   
   Base 256 sounds more like binary encoding; I'm not sure we gain much by allowing it as a textStandardBase as well. Eg. when the string "0" means 48, I do not think you are working with text.

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] bsloane1650 commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366114676
 
 

 ##########
 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:
   Aren't we also testing for this in ElementBaseGrammarMixin? Why do we need to check twice?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r369613489
 
 

 ##########
 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:
   You missed the "Just Kidding" at the end of my comment. :-)
   
   +1

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366360286
 
 

 ##########
 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:
   I think we should support base 256 also, where the charcodes 0 to 255 are the digits.
   
   Base e (2.71828...) has also been suggested somewhere.....
   
   Just kidding. 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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 @@
 
     <tdml:parserTestCase name="simple_type_properties_text_number_13_03" root="base_group"
 		model="./fvt/ext/dpa/dpanum_properties.dfdl.xsd" 
-		 description="Section 13.5 Specification of number base - 2,8, and 16">
+		 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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] codecov-io commented on issue #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#issuecomment-577260202
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-daffodil/pull/314?src=pr&el=h1) Report
   > Merging [#314](https://codecov.io/gh/apache/incubator-daffodil/pull/314?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-daffodil/commit/0ba8cfd1efb73be42d3ffb472dd26daf6b526946?src=pr&el=desc) will **increase** coverage by `0.06%`.
   > The diff coverage is `98.14%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-daffodil/pull/314/graphs/tree.svg?width=650&token=sXbCnhNQp8&height=150&src=pr)](https://codecov.io/gh/apache/incubator-daffodil/pull/314?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #314      +/-   ##
   ==========================================
   + Coverage   80.35%   80.41%   +0.06%     
   ==========================================
     Files         402      405       +3     
     Lines       23527    23576      +49     
     Branches     2762     2763       +1     
   ==========================================
   + Hits        18904    18958      +54     
   + Misses       4623     4618       -5
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-daffodil/pull/314?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...n/scala/org/apache/daffodil/dsom/ElementBase.scala](https://codecov.io/gh/apache/incubator-daffodil/pull/314/diff?src=pr&el=tree#diff-ZGFmZm9kaWwtY29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2RhZmZvZGlsL2Rzb20vRWxlbWVudEJhc2Uuc2NhbGE=) | `92.04% <ø> (ø)` | :arrow_up: |
   | [...rocessors/parsers/NonBaseTenTextNumberParser.scala](https://codecov.io/gh/apache/incubator-daffodil/pull/314/diff?src=pr&el=tree#diff-ZGFmZm9kaWwtcnVudGltZTEvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9kYWZmb2RpbC9wcm9jZXNzb3JzL3BhcnNlcnMvTm9uQmFzZVRlblRleHROdW1iZXJQYXJzZXIuc2NhbGE=) | `100% <100%> (ø)` | |
   | [...nparsers/ConvertNonBaseTenTextNumberUnparser.scala](https://codecov.io/gh/apache/incubator-daffodil/pull/314/diff?src=pr&el=tree#diff-ZGFmZm9kaWwtcnVudGltZTEtdW5wYXJzZXIvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9kYWZmb2RpbC9wcm9jZXNzb3JzL3VucGFyc2Vycy9Db252ZXJ0Tm9uQmFzZVRlblRleHROdW1iZXJVbnBhcnNlci5zY2FsYQ==) | `100% <100%> (ø)` | |
   | [...ar/primitives/PrimitivesNonBaseTenTextNumber.scala](https://codecov.io/gh/apache/incubator-daffodil/pull/314/diff?src=pr&el=tree#diff-ZGFmZm9kaWwtY29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2RhZmZvZGlsL2dyYW1tYXIvcHJpbWl0aXZlcy9QcmltaXRpdmVzTm9uQmFzZVRlblRleHROdW1iZXIuc2NhbGE=) | `100% <100%> (ø)` | |
   | [...affodil/schema/annotation/props/ByHandMixins.scala](https://codecov.io/gh/apache/incubator-daffodil/pull/314/diff?src=pr&el=tree#diff-ZGFmZm9kaWwtbGliL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvZGFmZm9kaWwvc2NoZW1hL2Fubm90YXRpb24vcHJvcHMvQnlIYW5kTWl4aW5zLnNjYWxh) | `76.27% <100%> (+5.1%)` | :arrow_up: |
   | [...che/daffodil/grammar/ElementBaseGrammarMixin.scala](https://codecov.io/gh/apache/incubator-daffodil/pull/314/diff?src=pr&el=tree#diff-ZGFmZm9kaWwtY29yZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2RhZmZvZGlsL2dyYW1tYXIvRWxlbWVudEJhc2VHcmFtbWFyTWl4aW4uc2NhbGE=) | `86.37% <85.71%> (+0.09%)` | :arrow_up: |
   | ... and [1 more](https://codecov.io/gh/apache/incubator-daffodil/pull/314/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-daffodil/pull/314?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-daffodil/pull/314?src=pr&el=footer). Last update [0ba8cfd...80e81f4](https://codecov.io/gh/apache/incubator-daffodil/pull/314?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366372672
 
 

 ##########
 File path: daffodil-test-ibm1/src/test/resources/test-suite/tresys-contributed/BG.tdml
 ##########
 @@ -25,25 +25,25 @@
     description="Text number properties">
 
 Review comment:
   75 or so tests did originate with IBM, which donated them to the DFDL Workgroup and the Daffodil effort 7 years back. 
   
   The AA-ZZ (aka the "two letter tests") originated with the original Daffodil NCSA code base before anybody including myself started working with this code. 
   
   So calling this bunch of tests "ibm1" is really misleading. Nevertheless keeping them distinct from the more organized and methodically created daffodil-test suite makes sense. 

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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 @@
 
     <tdml:parserTestCase name="simple_type_properties_text_number_13_03" root="base_group"
 		model="./fvt/ext/dpa/dpanum_properties.dfdl.xsd" 
-		 description="Section 13.5 Specification of number base - 2,8, and 16">
+		 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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366364645
 
 

 ##########
 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:
   While this is shorter, you are depending on the set of types not changing here. E.g., if a new type is introduced (quad-precision floats?) then this will break. 
   
   It is better to positively select the integer types that support this than use negation of the the floating point types that do not. 
   
   You can still do this without enumerating all the integer types because they're all subtypes of PrimType.Integer.Kind 
   
   `case _: PrimType.Integer.Kind => ...`

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] bsloane1650 commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366119214
 
 

 ##########
 File path: daffodil-test-ibm1/src/test/resources/test-suite/tresys-contributed/BG.tdml
 ##########
 @@ -25,25 +25,25 @@
     description="Text number properties">
 
 Review comment:
   We are making many changes to existins tests in the test-ibm1 project. Do we have compatability concerns to worry about?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [incubator-daffodil] bsloane1650 commented on a change in pull request #314: Add support for textStandardBase

Posted by GitBox <gi...@apache.org>.
bsloane1650 commented on a change in pull request #314: Add support for textStandardBase
URL: https://github.com/apache/incubator-daffodil/pull/314#discussion_r366117942
 
 

 ##########
 File path: daffodil-test-ibm1/src/test/resources/test-suite/ibm-contributed/dpaext2.tdml
 ##########
 @@ -145,7 +145,7 @@
 
     <tdml:parserTestCase name="simple_type_properties_text_number_13_03" root="base_group"
 		model="./fvt/ext/dpa/dpanum_properties.dfdl.xsd" 
-		 description="Section 13.5 Specification of number base - 2,8, and 16">
+		 description="Section 13.5 Specification of number base - 2,8, and 16" roundTrip="twoPass">
 
 Review comment:
   I would prefer to avoid having twoPass test cases, as they obscure what is actually going on. Can we instead update the document/infoset to match what we are expecting in the second pass. If we car about the details of the intermediate parses, it would be clearer to break this test into multiple tests that should the individual parse/unparse steps with roundTrip="None"

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services