tuxji commented on code in PR #1067:
URL: https://github.com/apache/daffodil/pull/1067#discussion_r1293592982
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -226,14 +226,30 @@ class CLIConf(arguments: Array[String], stdout:
PrintStream, stderr: PrintStream
implicit def rootNSConverter =
org.rogach.scallop.singleArgConverter[RefQName](qnameConvert _)
implicit def fileResourceURIConverter = singleArgConverter[URI]((s: String)
=> {
- val file = new File(s)
- val uri =
- if (file.isFile()) {
- Some(file.toURI)
- } else {
- Misc.getResourceRelativeOption(s, None)
+ val optURI = None
+ .orElse {
+ val file = new File(s)
+ if (file.isFile) Some(file.toURI)
+ else None
+ }
+ .orElse {
+ if (s.startsWith("/")) {
+ val resource = this.getClass.getResource(s)
+ Option(resource.toURI)
Review Comment:
If the resource does not exist or is not visible due to security
considerations, the `getResource` method returns null. Do you need a null
check before calling `resource.toURI` and another `None` if the null check
fails?
Also, lines 237 - 238 don't seem to be covered by tests.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -226,14 +226,30 @@ class CLIConf(arguments: Array[String], stdout:
PrintStream, stderr: PrintStream
implicit def rootNSConverter =
org.rogach.scallop.singleArgConverter[RefQName](qnameConvert _)
implicit def fileResourceURIConverter = singleArgConverter[URI]((s: String)
=> {
- val file = new File(s)
- val uri =
- if (file.isFile()) {
- Some(file.toURI)
- } else {
- Misc.getResourceRelativeOption(s, None)
+ val optURI = None
+ .orElse {
+ val file = new File(s)
+ if (file.isFile) Some(file.toURI)
+ else None
+ }
+ .orElse {
+ if (s.startsWith("/")) {
+ val resource = this.getClass.getResource(s)
+ Option(resource.toURI)
+ } else {
+ None
+ }
+ }
+ .orElse {
+ val resource = this.getClass.getResource("/" + s)
+ if (resource != null) {
+ Logger.log.warn(s"Found relative path on classpath absolutely, did
you mean /$s")
+ Some(resource.toURI)
Review Comment:
If the previous `.orElse` code returns None because a resource cannot be
found when s starts with "/", this code will add another "/" to s, still fail
to find the resource, and return None again.
Should we combine these two `.orElse` cases by assigning an if expression to
`val resource`?
```scala
val resource = if (s.startsWith("/")) s else "/" + s
```
Actually, you want to warn the user if a relative path is found absolutely
on the classpath, but you can guard the warning with an `if
(!s.startsWith("/"))`.
Line 247 doesn't seem to be covered by tests too.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/xml/DaffodilXMLLoader.scala:
##########
@@ -251,26 +240,32 @@ class DFDLCatalogResolver private ()
None
}
case (null, sysId) => {
- // We did not get a catalog resolution of the nsURI, nor
- // a straight file resolution of the systemId, so we
- // use the systemId (which comes from the schemaLocation attribute)
- // and the classpath.
- val baseURI = if (baseURIString == null) None else Some(new
URI(baseURIString))
- val optURI = Misc.getResourceRelativeOption(sysId, baseURI)
+ // We did not get a catalog resolution of the nsURI. We now look for
the systemID (which
+ // comes from the schemaLocation attribute) on classpath or as a file.
Note that we
+ // ignore if boolean
Review Comment:
The sentence `Note that we ignore if boolean` is incomplete and I don't know
what you mean by it. Do you mean `Note that we ignore sysId's boolean value.`?
##########
daffodil-tdml-processor/src/test/scala/org/apache/daffodil/processor/tdml/TestDaffodilC.scala:
##########
@@ -137,7 +137,7 @@ class TestDaffodilC {
val input = new ByteArrayInputStream(b)
val pr = tdp.parse(input, b.length * 8)
assert(
- !pr.isProcessingError && pf.getDiagnostics.isEmpty,
+ !pr.isProcessingError,
Review Comment:
Let's discuss whether to remove `&& pf.getDiagnostics.isEmpty` or not. When
we compile the generated C code into an executable and run that executable, the
program will invariably print validation diagnostics if it detects validation
problems like an element's value not matching its fixed attribute. However,
the program returns an error exit status only if you pass `-V on` or `-V
limited`, otherwise it returns a successful exit status. I wanted to fail
these test cases and print the validation diagnostics if any are printed.
Note that right now, the TDML Runner doesn't pass any validation option to
the daffodilC executable, but the TDML Runner does return any printed messages
as validation diagnostics in the TDML result, so that the TDML Runner will do
the right thing with any validation error clauses in the test cases.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -226,14 +226,30 @@ class CLIConf(arguments: Array[String], stdout:
PrintStream, stderr: PrintStream
implicit def rootNSConverter =
org.rogach.scallop.singleArgConverter[RefQName](qnameConvert _)
implicit def fileResourceURIConverter = singleArgConverter[URI]((s: String)
=> {
- val file = new File(s)
- val uri =
- if (file.isFile()) {
- Some(file.toURI)
- } else {
- Misc.getResourceRelativeOption(s, None)
+ val optURI = None
Review Comment:
This None.orElse{}... expression evaluates each alternative in turn,
stopping when one returns a non-empty Option.
If you think a reader might appreciate the above line as a comment, please
add it. I actually had to go look at the definition of .orElse in IDEA.
--
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]
