Remove uses of ReflectiveCalls from Code#1452
Conversation
olabusayoT
changed the title
mbeckerle
left a comment
mbeckerle
left a comment
There was a problem hiding this comment.
+1. Suggest minor name changes. Trait name should definitely begin with uppercase letter, but the other stuff is just suggestions.
| abstract class BitsCharsetDefinition( | ||
| charset: BitsCharset, | ||
| alias: Option[String] = None | ||
| ) extends hasNameDefined { |
There was a problem hiding this comment.
Should be "H" initial caps on trait name.
I suggest remove the suffix "Defined" and just call it "HasName" or "HasNameMethod"
stevedlawrence
left a comment
stevedlawrence
left a comment
There was a problem hiding this comment.
+1 with some suggestiosn
| import scala.collection.mutable.ArrayBuffer | ||
| import scala.language.reflectiveCalls | ||
|
|
||
| trait hasNameDefined { def name(): String } |
There was a problem hiding this 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.
| * into it for Java users who want JDOM. | ||
| */ | ||
| def Infoset2JDOM(node: { def toXML: scala.xml.NodeSeq }) = { | ||
| def Infoset2JDOM(node: XMLConvertible) = { |
There was a problem hiding this 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.
| * 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 = |
There was a problem hiding this 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.
- remove need for reflection by using traits instead of structural typing since scala 3 has a different import scala.reflect.Selectables... from scala 2.13 which uses scala.language.reflectiveCalls. Neither import exist in the other version and are therefore incompatible, so the easiest approach was to get rid of structural typing in lieu of traits. - add comments for 2.12 phaseout - remove JDOMUtils DAFFODIL-2975
olabusayoT
force-pushed
the
daf-2975-remove-structural-typing
branch
from
c1d8e9a to
75f6c5e
Compare
3 participants
DAFFODIL-2975