stevedlawrence commented on code in PR #1452:
URL: https://github.com/apache/daffodil/pull/1452#discussion_r1986045732
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/JDOMUtils.scala:
##########
@@ -21,17 +21,20 @@ import scala.xml._
import org.apache.daffodil.lib.exceptions.Assert
+// For use as a type bound instead of a structural type so we don't depend on
reflection
+trait XMLConvertible {
+ def toXML: scala.xml.NodeSeq
+}
+
object JDOMUtils {
val xsiNS = org.jdom2.Namespace.getNamespace("xsi",
XMLUtils.XSI_NAMESPACE.toString)
- import scala.language.reflectiveCalls // allows structural type - the def
toXML below.
-
/**
* JDOM is no longer our infoset, but we will still want to convert
* into it for Java users who want JDOM.
*/
- def Infoset2JDOM(node: { def toXML: scala.xml.NodeSeq }) = {
+ def Infoset2JDOM(node: XMLConvertible) = {
Review Comment:
This function isn't used anymore. I would suggest we just remove it. If
something wants to do this it can just do `JDOMUtils.elem2Element(foo.toXML)`
itself.
In fact, I wonder if we should just remove JDOMUtils entirely? It looks like
this was used before we had native JDOM support via the
JDOMInfosetInputter/Outputter, and so we would output the infoset as ScalaXML
nodes and then convert that to JDOM when a user want that. That's pretty
inefficient which is why we don't do it anymore. And now nothing actually uses
it except for one test that checks that `xsi:nil="true"` works correctly. With
such minimal test coverage and nothing using it, it wouldn't surprise me if it
doesn't even work anymore or there's some edge case we never considered and
haven't found because nothing uses it. Looking at the git logs, its last use
was 2017--something that goes unused for that long should probably be removed.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/SimpleNamedServiceLoader.scala:
##########
@@ -19,7 +19,8 @@ package org.apache.daffodil.lib.util
import java.util.ServiceConfigurationError
import java.util.ServiceLoader
import scala.collection.mutable.ArrayBuffer
-import scala.language.reflectiveCalls
+
+trait hasNameDefined { def name(): String }
Review Comment:
Thoughts on renaming this to something like `SimpleNamedLoadableService` or
`SimpleNamedService`? It's only used by our service loader classes, so this
makes it more obvious which classes are plugins. And if we ever want to require
that our simple service loader classes implement new fields other than just
`name`, then we can add it to this trait and its name still makes sense.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -638,11 +638,10 @@ object Misc {
}
}
- import scala.language.reflectiveCalls // scala 2.10 creates warning unless
we have this.
/**
* Convenient I/O tools
*/
- def using[A <: { def close(): Unit }, B](param: A)(f: A => B): B =
+ def using[A <: AutoCloseable, B](param: A)(f: A => B): B =
Review Comment:
Scala 2.13 adds a new `scala.util.Using` object that looks like a drop-in
replacement: https://www.scala-lang.org/api/2.13.6/scala/util/Using$.html. It
might be worth switching to that once we drop 2.12 support. It has a bit more
features than we probably need, but it's one less thing we need to maintain.
Also, it looks like we have two duplicate implementations of `using`, one
here in Misc and one in Implicits. We may want to consider removing the
Implicits one (there isn't anything actually implicit about it). Though if the
plan is to replace `using` with `scala.util.Using` maybe we just wait until
that.
--
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]
