[GitHub] [daffodil] stevedlawrence commented on a diff in pull request #799: DAFFODIL-2369 Any character following "//" will be ignored in ByteDocumentPart/BitDocumentPart

[GitBox <gi...@apache.org>]

stevedlawrence commented on code in PR #799:
URL: https://github.com/apache/daffodil/pull/799#discussion_r886044679


##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -2364,6 +2361,35 @@ sealed abstract class DocumentPart(part: Node, parent: Document) {
     }
   }.trim.toUpperCase()
 
+  /*
+  * Description: Compare ByteDocumentPart/BitsDocumentPart string to a string of valid characters.
+  *              If the string runs into non-valid characters, skip over them.
+  *              If the string runs into "//", any character following it will be skipped (including valid ones).
+  *              Characters on a new line will be considered again, unless they are found after a "//"
+  * Parameters: String containing the acceptable characters (0,1 for bits) (0-9,A-F,a-f for bytes)

Review Comment:
   I'd also suggest we pass in the string to remove comments from (e.g. rawPartContent), rather than requiring that it be a member of the class that the function accesses. That way this is more of a utility function (and could even go in some Utils object) and not something specific to certain classes that need a certain member variable.



##########
daffodil-tdml-lib/src/main/scala/org/apache/daffodil/tdml/TDMLRunner.scala:
##########
@@ -2364,6 +2361,35 @@ sealed abstract class DocumentPart(part: Node, parent: Document) {
     }
   }.trim.toUpperCase()
 
+  /*
+  * Description: Compare ByteDocumentPart/BitsDocumentPart string to a string of valid characters.
+  *              If the string runs into non-valid characters, skip over them.
+  *              If the string runs into "//", any character following it will be skipped (including valid ones).
+  *              Characters on a new line will be considered again, unless they are found after a "//"
+  * Parameters: String containing the acceptable characters (0,1 for bits) (0-9,A-F,a-f for bytes)
+  * Return: ListBuffer[Char] of the accepted characters from the ByteDocumentPart/BitsDocumentPart element
+  */
+  def commentCompatibleFormat(validCharacters: String) : ListBuffer[Char] = {
+    val tempListBuffer = new ListBuffer[Char]()
+    var noCommentBool = true
+
+    for (i <- 0 to partRawContent.length()-1){

Review Comment:
   >  Then replace any char matching regular expression "[^0-9a-fA-F]" with "". (for bytes case)
   
   Should we consider that an error rather than replacing invalid characters with empty string? Isn't the main purpose of the comment syntax to avoid cases where we accidentally misinterpret characters? I suggest any characters not in a comment *must* be a valid character for that the document type, or a space to allow for alignment/readability. Any other character should throw a TDMLException about an invalid document part.



##########
daffodil-tdml-lib/src/test/scala/org/apache/daffodil/tdml/UnitTestTDMLRunner.scala:
##########
@@ -345,6 +345,46 @@ class UnitTestTDMLRunner {
     assertEquals(expected, actual)
   }
 
+  @Test def testCommentBit(): Unit = {
+        val xml = <document bitOrder="LSBFirst">
+                <documentPart type="bits">00000010 //this is a label111</documentPart>

Review Comment:
   The syntax highlighting here is a little odd because // is a scala comment. I wonder if that could cause confusion in IDEs? Maybe we should use a comment character that is unique and doesn't necessarily mean anything special in scala as far as syntax highlighting goes?
   
   Also, why did we decide not to use XML comments? Syntax highlighting would just work, and they should be pretty straightforward to remove using normal XML utilities. Is that just too verbose?



-- 
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: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org