[GitHub] [daffodil] tuxji commented on a diff in pull request #886: Update scaladocs mentioning deprecated API (also fix CI)

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

tuxji commented on code in PR #886:
URL: https://github.com/apache/daffodil/pull/886#discussion_r1042394559


##########
daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/DataProcessor.scala:
##########
@@ -154,27 +154,23 @@ class DataProcessor private (
    * That means when we save for reloading, we must explicitly clobber validationMode and externalVars to
    * initialized values.
    *
-   * @throws java.io.ObjectStreamException
+   * @throws java.io.ObjectStreamException Must be part of writeReplace's API
    * @return the serializable object
    */
   @throws(classOf[java.io.ObjectStreamException])
   private def writeReplace() : Object =
     new SerializableDataProcessor(ssrd, tunables, externalVars, validationMode)
 
   /**
-   * The compilerExternalVars argument supports the deprecated feature to assign external var bindings
-   * on the compiler object.
-   *
-   * These are just incorporated into the initial variable map of the data processor.
+   * This constructor reconstructs a DataProcessor from a SerializedDataProcessor.
    */
-
   def this(
     ssrd: SchemaSetRuntimeData,
     tunables:DaffodilTunables,
-    compilerExternalVars: Queue[Binding] = Queue.empty,
+    externalVars: Queue[Binding] = Queue.empty,
     validationMode: ValidationMode.Type = ValidationMode.Off) =
-    this(ssrd, tunables, ExternalVariablesLoader.loadVariables(compilerExternalVars, ssrd, ssrd.originalVariables),
-      false, None, validationMode, compilerExternalVars)
+    this(ssrd, tunables, ExternalVariablesLoader.loadVariables(externalVars, ssrd, ssrd.originalVariables),
+      false, None, validationMode, externalVars)

Review Comment:
   I had initially thought SchemaSetRuntime1Mixin.scala was the only caller of this constructor too and removed the externalVars parameter from this constructor, but I got a compilation error which showed that there was a second call of this constructor from SerializableDataProcessor's constructor in DataProcessor.scala at line 107 that I didn't know about.  SerializableDataProcessor extends DataProcessor(data, tunable, externalVars, validationModeArg) in order to preserve the externalVars and validationMode in case they may be needed by other serializations besides Daffodil save/reload such as Apache Spark which serializes in order to move objects for remote execution.  
   
   We can't remove the externalVars field from DataProcessor because we inform DataProcessor about external variable bindings in that field.  DataProcessor's primary constructor is private (probably because of the mix of parameters some of which should be serialized all the time, some which should be serialized only sometimes, and some which should never be serialized).  DataProcessor has both a class and an object, but the object DataProcessor has no apply method, only a private class SerializableDataProcessor.  Therefore, this particular (public) constructor is the only way which callers outside DataProcessor.scala can create a DataProcessor.  Our options are to keep this public constructor or add an apply method to the object DataProcessor which calls the private constructor.
   
   At the very least, I need to fix this constructor's scaladoc.  I had changed the scaladoc to say it was needed by SerializableDataProcessor, but I misstated the reason.  I can make the scaladoc state that this constructor exists to construct a SerializableDataProcessor, not the other way around.  Or I can remove this constructor and add an apply method to object DataProcessor, which would put that apply method next to the class SerializableDataProcessor and make the relationship between them more obvious.  I would also add a second apply method with only 2 parameters and its own scaladoc to let external callers like SchemaSetRuntime1Mixin.scala create a DataProcessor.
   
   Thoughts?



-- 
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