stevedlawrence commented on code in PR #1315:
URL: https://github.com/apache/daffodil/pull/1315#discussion_r1775311934
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -1012,30 +1033,41 @@ class Main(
}
}
- def withDebugOrTrace(proc: DFDL.DataProcessor, conf: CLIConf):
DFDL.DataProcessor = {
- (conf.trace() || conf.debug.isDefined) match {
+ def withDebugOrTrace(
+ proc: DFDL.DataProcessor,
+ traceArg: ScallopOption[Boolean],
+ debugArg: ScallopOption[Option[String]]
Review Comment:
Indention got weird here. scalafmt should fix it?
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -617,7 +629,7 @@ class CLIConf(arguments: Array[String], stdout:
PrintStream, stderr: PrintStream
// Test Subcommand Options
object test extends scallop.Subcommand("test") {
banner(
- """|Usage: daffodil test [-I <implementation>] [-l] [-r] [-i] <tdmlfile>
[testnames...]
+ """|Usage: daffodil test [-I <implementation>] [-l] [-r] [-i] [-d <file>
| -d -- | -t] <tdmlfile> [testnames...]
Review Comment:
`-d --` isn't really the right way to signify the ability to use `--`, since
it sort of implies `--` is an argument to the `-d` option. But that's not
really case. The `--` can go anywhere and just says that everything following
it should not be treated as an option or an option argument, and should instead
be treated as a trailing argument.
We happen to only really need it for the -d option since there's sometimes
an ambiguity if the thing following it is the file with debug commands or a
trailing argument. This ambiguity is whys why it's needed in some of our tests
that do something like this:
```
daffodil test -d foo.tdml`
```
In this case you do need the `--` to say that foo.tdml is not actually an
argument to the -d option, e.g.
```
daffodil test -d -- foo.tdml
```
But note that `--` isn't always necessary if there is no ambiguity. For
example, if you do this
```
daffodil test -d -r foo.tdml 'test_foo_.*'
```
Then scallop knows that `-r` is an option so it can infer the `-d` option
did not have an argument, and so the `--` isn't needed at all. This is also why
you don't need `-d --` for the parse/unparse tests. In those tests there is no
ambiguity because `-d` is immediately followed by another option.
But note that the same problem can still happen for the parse/unparse
commands, it's not specific to the test command. For example, if you did this:
```
daffodil parse -s foo.dfdl.xsd -d input.bin
```
Scallop would think "input.bin" is the file of debug commands, there would
be no trailing args, and so daffodil would parse stdin. Instead, you would need
to do:
```
daffodil parse -s foo.dfdl.xsd -d -- input.bin
```
The `--` is also useful in rare edge cases where the file you want to parse
looks like an argument. For example, if you had a file that started with a
hyphen you would also need to use `--`, e.g.
```
daffodil parse -s foo.dfdl.xsd -- --file_that_looks_like_an_argument.bin
```
(there's good reason we don't start files with hyphens).
So all this to say that `--` isn't really specific to `-d` so shouldn't be
added to the usage like that. If we really want to make it clear that it can be
used, probably the best way to do it is to add `[--]` before the trailing
arguments. That would imply that `--` is optional, but if you want it to deal
with ambiguity then put it before trailing arguments.
Git does something similar for commands like `git add`, and the man page
even says:
```
--
This option can be used to separate command-line options from the
list of files, (useful when filenames might be mistaken for command-line
options).
```
I'm not sure if scallop would let us add that to the option description
though.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -453,6 +456,7 @@ class CLIConf(arguments: Array[String], stdout:
PrintStream, stderr: PrintStream
banner("""|Usage: daffodil unparse (-s <schema> [-r <root>] | -P <parser>)
| [-c <file>] [-D<variable>=<value>...]
[-I <infoset_type>]
| [-o <output>] [--stream]
[-T<tunable>=<value>...] [-V <mode>]
+ | [--trace | --debug <file>]
| [infile]
Review Comment:
These usage strings are starting to get a bit verbose. I'm wondering they
are getting too complicated to be useful and if we should simplify the usage
string to just something like this:
```
Usage: daffodil unparse (-s <schema> | -P <parser>) [UNPARSE_OPTS] [--]
[infile]
Unparse Options:
-- scallop generated stuff--
```
So it shows only the required and commonly used options (-s and -P) and
trailing args, and anything else is described in more detail below that. We
would probably want to also modify some descriptions to mention restrictions.
For example, '--root' would need to say that it's only valid with the --schema
option. But that and trace/debug mutual exclusion are the only restrictions I
think.
It also makes the `[--]` stand out a bit more so might make it's
availability more clear. And it means we don't have to update this usage and
make it even more complicated every time we add a new option.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -337,6 +331,7 @@ class CLIConf(arguments: Array[String], stdout:
PrintStream, stderr: PrintStream
banner("""|Usage: daffodil parse (-s <schema> [-r <root>] | -P <parser>)
| [-c <file>] [-D<variable>=<value>...] [-I
<infoset_type>]
| [-o <output>] [--stream]
[-T<tunable>=<value>...] [-V <mode>]
+ | [--trace | --debug <file>]
Review Comment:
`<file>` should be in brackets to imply it's optional, e.g. `[--trace |
--debug [<file>]]`. Same for the unparse command.
##########
daffodil-cli/src/main/scala/org/apache/daffodil/cli/Main.scala:
##########
@@ -638,12 +650,22 @@ class CLIConf(arguments: Array[String], stdout:
PrintStream, stderr: PrintStream
tally(descr = "Increment test result information output level, one level
for each -i")
val list = opt[Boolean](descr = "Show names and descriptions instead of
running test cases")
val regex = opt[Boolean](descr = "Treat <testnames...> as regular
expressions")
+ val debug = opt[Option[String]](
+ argName = "file",
+ descr =
+ "Enable the interactive debugger. Optionally, read initial debugger
commands from [file] if provided. " +
+ "To use debug without a file, one must supply '--' inplace of file "
+
+ "i.e 'daffodil test -d -- path/to/tests.tdml'"
Review Comment:
This description isn't quite right. See above comment.
--
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]
