mbeckerle commented on code in PR #1065:
URL: https://github.com/apache/daffodil/pull/1065#discussion_r1284566724
##########
daffodil-cli/src/test/scala/org/apache/daffodil/cli/cliTest/TestCLISaveParser.scala:
##########
@@ -318,4 +318,18 @@ class TestCLISaveParser {
}
}
+ @Test def test_CLI_Saving_SaveParser_error(): Unit = {
Review Comment:
Please add scaladoc explaining what this test does. It's a bit opaque just
looking at it.
Does it make sure "~/" is in messages instead of long absolute paths?
Does it matter where the user has daffodil installed? Or whatever DFDL
schema they are working on?
What if those (or any one) are not under their home directory at all?
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/api/Diagnostic.scala:
##########
@@ -263,7 +263,12 @@ trait LocationInSchemaFile {
): String = {
// works for both windows and unix
val splitter = "/"
- val tokens = uriString.split(splitter)
+ val classpathTrimmedUriString =
+ uriString.replace(
+ Misc.classPath.find(cp =>
uriString.startsWith(cp.toString)).getOrElse("").toString,
Review Comment:
Same idiom. I found confusing above.
Suggest create a private helper method and call it in both places. That way
you can explain it once.
Name it "stripClasspathPrefix" or something like that perhaps.
##########
daffodil-test/src/test/resources/org/apache/daffodil/section02/schema_definition_errors/SchemaDefinitionErrors.tdml:
##########
@@ -298,6 +298,30 @@
</tdml:parserTestCase>
+ <tdml:defineConfig name="cfg_maxParentDirectoriesForDiagnostics_3">
+ <daf:tunables xmlns="http://www.w3.org/2001/XMLSchema";
+ xmlns:xs="http://www.w3.org/2001/XMLSchema";>
+
<daf:maxParentDirectoriesForDiagnostics>20</daf:maxParentDirectoriesForDiagnostics>
Review Comment:
Need a test where you clamp this to 1. Maybe that's in another PR already
merged? I don't see the definition of this new tunable in this PR, so I'm
guessing that is already covered. If not add a test to that effect.
##########
daffodil-lib/src/main/scala/org/apache/daffodil/lib/util/Misc.scala:
##########
@@ -660,4 +660,15 @@ object Misc {
}
Some(res)
}
+
+ /**
+ * Replace HOME env var in Path with ~
+ */
+ def depersonalizePath(path: String): String = {
+ path.replace(
+ System.getenv("HOME"),
Review Comment:
HOME isn't defined on MS-Windows. This is linux/unix specific.
According to ChatGPT, this is the portable way to do this:
```
String homeDir = System.getProperty("user.home");
```
##########
daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/DFDLSchemaFile.scala:
##########
@@ -137,9 +145,29 @@ final class DFDLSchemaFile(
override lazy val optXMLSchemaDocument = iiParent.optXMLSchemaDocument
- override lazy val uriString = schemaSource.uriForLoading.toString
+ override lazy val uriString = {
+ val uriString_ = schemaSource.uriForLoading.toString
+ val finalUriString = Misc.depersonalizePath(
+ uriString_
+ .replace(
+ Misc.classPath.find(cp =>
uriString_.startsWith(cp.toString)).getOrElse("").toString,
+ "",
+ ),
+ )
+ finalUriString
+ }
- override protected lazy val diagnosticDebugNameImpl =
schemaSource.uriForLoading.toString
+ override protected lazy val diagnosticDebugNameImpl = {
+ val uriString_ = schemaSource.uriForLoading.toString
+ val finalUriString = Misc.depersonalizePath(
+ uriString_
+ .replace(
+ Misc.classPath.find(cp =>
uriString_.startsWith(cp.toString)).getOrElse("").toString,
Review Comment:
This code is confusing.
Can you spread this code out more? Add an intermediate for the uriString
where the prefix that is found on the classpath has been removed, then
depersonalize that in a separate line? The cascade of the replace with the find
with the getOrElse is not easily understood.
--
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]
