tuxji commented on code in PR #1176: URL: https://github.com/apache/daffodil/pull/1176#discussion_r1519192538
########## daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/layers/api/Layer.java: ########## @@ -0,0 +1,134 @@ +/* + * 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.runtime1.layers.api; + +import org.apache.daffodil.runtime1.layers.LayerUtils; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; + +/** + * This is the primary API class for writing layers. + * <p/> + * All layers are derived from this class, and must have no-args default constructors. + * <p/> + * Derived classes will be dynamically loaded by Java's SPI system. + * The names of concrete classes derived from Layer are listed in a resources/M.services file Review Comment: *resources/META-INF/services ########## daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SequenceGroup.scala: ########## @@ -360,6 +342,7 @@ final class ChoiceBranchImpliedSequence private (rawGM: Term) override def separatorPosition: SeparatorPosition = SeparatorPosition.Infix override def isLayered: Boolean = false + override def layerOption: Option[RefQName] = None Review Comment: My browser sees only 4 places where `layerOption` occurs in this GitHub page (even after loading all the diffs hidden from view). Of these 4 places, this appears to be the only place which initializes `layerOption` to anything, and it's initializing `layerOption` to `None`. A codecov warning also says this line is not called, so where else is the codebase actually setting `layerOption` to anything at all? ########## daffodil-test/src/test/scala/org/apache/daffodil/runtime1/layers/CheckDigitLayer.scala: ########## @@ -47,33 +47,37 @@ object CheckDigitLayer { * - digitEncoding is a string which is the name of a character set encoding which * defines the character set of the digits. * This must match the dfdl:encoding for the DFDL element declaration using this layer. - * - * @param length the value of the length DFDL variable - * @param params the value of the params DFDL variable - * @param digitEncoding the value of the digitEncoding DFDL variable */ -class CheckDigitLayer(length: JInt, params: String, digitEncoding: String) +class CheckDigitLayer extends ChecksumLayer( "checkDigit", // name of the layer, also happens to be the name of the output variable. CheckDigitLayer.ns, - length, ) { + private var params: String = _ + private var digitEncoding: String = _ + private lazy val charset = Charset.forName(digitEncoding) + private lazy val isVerbose = params.toLowerCase.contains("verbose") + + /** + * @param length the value of the length DFDL variable + * @param params the value of the params DFDL variable + * @param digitEncoding the value of the digitEncoding DFDL variable + */ + def setLayerVariableParameters(length: Int, params: String, digitEncoding: String): Unit = { + setLength(length) + this.params = params + this.digitEncoding = digitEncoding + } + /** * Result DFDL variable value getter. This getter is called to obtain the value for, * and populate the DFDL variable named checkDigit, in the layer's target namespace. * @return the check digit value */ - def getDFDLResultVariable_checkDigit: JInt = + def getLayerVariableResult_checkDigit: JInt = Review Comment: Question has not been resolved yet. I got this far only by skimming; I didn't have time to review 2/3s of the PR thoroughly. I will accept that the changes work since the tests pass. ########## daffodil-core/src/test/scala/org/apache/daffodil/core/layers/TestLayers.scala: ########## @@ -141,18 +200,64 @@ a few lines of pointless text like this.""".replace("\r\n", "\n").replace("\n", assertEquals(text, textBack) } + val GZIPLayer0Schema = + SchemaUtils.dfdlTestSchema( + <xs:include schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/> + <xs:import namespace="urn:org.apache.daffodil.layers.gzip" + schemaLocation="/org/apache/daffodil/layers/xsd/gzipLayer.dfdl.xsd"/>, + <dfdl:format ref="ex:GeneralFormat" representation="binary"/>, + <xs:element name="e1" dfdl:lengthKind="implicit" + xmlns:gz="urn:org.apache.daffodil.layers.gzip"> + <xs:complexType> + <xs:sequence dfdlx:layer="gz:gzip"> + <xs:element name="s1" type="xs:string" dfdl:lengthKind="delimited"/> + </xs:sequence> + </xs:complexType> + </xs:element>, + elementFormDefault = "unqualified", + ) + + def makeGZIPLayer0Data() = { + val gzipData: Array[Byte] = makeGZIPData(text) + gzipData + } + + @Test + def testGZIPLayer0(): Unit = { + val sch = GZIPLayer0Schema + val data = makeGZIPLayer0Data() + val infoset = <ex:e1 xmlns:ex={example}><s1>{ + text + }</s1></ex:e1> + val (_, actual) = TestUtils.testBinary(sch, data, areTracing = false) + TestUtils.assertEqualsXMLElements(infoset, actual) + + TestUtils.testUnparsingBinary(sch, infoset, data) Review Comment: I noticed that there are not pairs of assertEquals on both the infoset and the data like I would expect, but it turns out that the `TestUtils.testUnparsingBinary` has its own assertEquals checking that the data round trips back to the same data. ########## daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SequenceGroup.scala: ########## @@ -277,9 +259,9 @@ trait SequenceDefMixin protected def emptyFormatFactory = new DFDLSequence(newDFDLAnnotationXML("sequence"), this) - final lazy val <sequence>{apparentXMLChildren @ _*}</sequence> = (xml \\ "sequence")(0) + final lazy val <sequence>{apparentXMLChildren @ _*}</sequence> = (xml \\ "sequence").head Review Comment: A comment might be nice to explain this unusual syntax. I normally expect the "final lazy val" to be followed by an identifier, but I see what appears to be a pattern deconstruction expression instead. I believe that the pattern expression on the left side is deconstructing the XML Node on the right side, stripping off the `<sequence>...</sequence>` tags and initializing `apparentXMLChildren` with whatever was between the `<sequence>...</sequence>` tags. Correct? I also suppose that if `xml` doesn't contain a `<sequence>` node as the parent node or as a child node below the parent node (which one is `xml \\ "sequence"` looking for?), then either the DPath expression or the pattern deconstruction expression will fail and throw an exception. Is the exception caught higher up and turned into a schema definition error? -- 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]
