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

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

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


##########
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 wonder if this constructor should even exist? Or if it should be changed to not even accept `externalVars` and `validationMode`? I think those parameters should never be passed in now, with the only way to change them via the `withXYZ` functions on the `DataProcessor`?
   
   I *think* the only use of this constructor is in SchemaSetRuntime1Mixin.scala, and it doesn't pass anything in anything for these parameters. Seems like maybe this constructor can be removed, and the SSRD can be responsible for creating the variable map before it creates the `DataProcessor` and passing in the other variables that it currently doesn't explicitly set? Or maybe just remove these params so they can't be passed in?



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