stevedlawrence commented on code in PR #116:
URL: https://github.com/apache/daffodil-sbt/pull/116#discussion_r2245562736
##########
src/main/scala/org/apache/daffodil/DaffodilPlugin.scala:
##########
@@ -458,7 +461,8 @@ object DaffodilPlugin extends AutoPlugin {
// where API functions to use. This is the mapping for which
Daffodil API should be
// used for a particular "internal API"
val daffodilInternalApiVersionMapping = Map(
- ">3.8.0" -> "2",
+ ">=4.0.0" -> "3",
+ "<=3.11.0 >3.8.0" -> "2",
Review Comment:
Following the convention of the other ranges in this plugin, this would be
`>=3.9.0 <=3.11.0 -> "2"`
##########
src/main/scala/org/apache/daffodil/DaffodilPlugin.scala:
##########
@@ -355,8 +355,11 @@ object DaffodilPlugin extends AutoPlugin {
"<=3.10.0" -> "2.12"
Review Comment:
Above we havea comment that says "the daffodil-japi dependency ....". Since
it could be daffodil-core, we should update it to something like "the Daffodil
dependency needed for daffodilPackageBin must ignore the scalaVersion ..."
##########
src/main/java/org/apache/daffodil/DaffodilSaver.java:
##########
@@ -147,14 +159,16 @@ private static void run(String[] args) throws Exception {
schemaArg = schemaUrl.toURI();
break;
case 2:
+ case 3:
schemaArg = schemaResource;
break;
}
Object processorFactory = compilerCompile.invoke(compiler, schemaArg,
root, null);
// val processorFactoryDiags = processorFactory.getDiagnostics()
List<?> processorFactoryDiags = (List<?>)
processorFactoryGetDiagnostics.invoke(processorFactory);
- printDiagnostics(processorFactoryDiags);
+ Class<?> diagnosticClass = Class.forName(baseApiPackage + ".Diagnostic");
Review Comment:
Can we move this at the to where we do all the other forName lookups. Makes
it easier to see all the API functions that DaffodilSaver uses. It might be
nice to also do the same for getMessage(isError) and getMessage(toString) and
pass those into print diagnostics too.
##########
src/main/scala/org/apache/daffodil/DaffodilPlugin.scala:
##########
Review Comment:
The `daffodilToScalaVersion` Map currently has `">=4.0.0" -> "3.3.5"`, but
since that's been added the Daffodil 4.0.0 main branch has updated to scala
3.3.6. We should update this to match. We should also remember to keep an eye
on this incase 3.3.7 is released and we update 4.0.0 to use that.
##########
src/main/scala/org/apache/daffodil/DaffodilPlugin.scala:
##########
@@ -355,8 +355,11 @@ object DaffodilPlugin extends AutoPlugin {
"<=3.10.0" -> "2.12"
)
val crossVersion = filterVersions(binDaffodilVersion,
daffodilToCrossVersion).head
- val dafDep = ("org.apache.daffodil" % "daffodil-japi" %
binDaffodilVersion % cfg)
- .withCrossVersion(CrossVersion.constant(crossVersion))
+ val dafDep = {
+ val dep = if (crossVersion == "3") ("org.apache.daffodil" %
"daffodil-core" % binDaffodilVersion % cfg)
+ else ("org.apache.daffodil" % "daffodil-japi" %
binDaffodilVersion % cfg)
+ dep.withCrossVersion(CrossVersion.constant(crossVersion))
+ }
Review Comment:
Maybe we should use the filterVersion thing for these deps. So replace
daffodilToCrossVersion with something like this:
```scala
val daffodilToPackageBinDep = Map(
">=4.0.0 " -> "org.apache.daffodil" % "daffodil-core" %
(binDaffodilVersion + "_3") % cfg,
"=3.11.0 " -> "org.apache.daffodil" % "daffodil-japi" %
(binDaffodilVersion + "_2.13") % cfg,
"<=3.10.0" -> "org.apache.daffodil" % "daffodil-japi" %
(binDaffodilVersion + "_2.12") % cfg
)
val dafDep = filterVersion(binDaffodilVersion, daffodilToPackageBinDep)
...
```
That feels a bit cleaner to me, and is also more easily extendable in the
future if we need to add additional version specific dependencies. It also
feels a bit easier to see exactly what dependency different versions of
daffodil use.
--
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]
