stevedlawrence commented on code in PR #1540:
URL: https://github.com/apache/daffodil/pull/1540#discussion_r2288609706
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Daffodil.java:
##########
@@ -224,6 +230,26 @@ public static InputSourceDataInputStream
newInputSourceDataInputStream(byte[] ar
return org.apache.daffodil.io.InputSourceDataInputStream.apply(arr);
}
+ /**
+ * Factory method to get an instance of InteractiveDebugger
+ *
+ * @param dr debugger runner
+ * @return instance of InteractiveDebuggerRunnerImpl
Review Comment:
I don't think we should mention InteractiveDebugger or
InteractiveDebuggerRunnerImpl, since that is internal details the public API
users don't need to know about. I think the documentation whats be something
like
> Factory method to get a Debugger that is controlled by an
InteractiveDebuggerRunner.
I'm also wondering if there's a better word to use than "Interactive".
Interactive kind of implies there some CLI or some user interacting with it,
but as a counter example the TraceDebuggerRunner doesn't have any user
interaction at all. I'm not sure what would be better, so I'm not sure it's a
big deal to change, just a though.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/Daffodil.java:
##########
@@ -224,6 +230,26 @@ public static InputSourceDataInputStream
newInputSourceDataInputStream(byte[] ar
return org.apache.daffodil.io.InputSourceDataInputStream.apply(arr);
}
+ /**
+ * Factory method to get an instance of InteractiveDebugger
+ *
+ * @param dr debugger runner
+ * @return instance of InteractiveDebuggerRunnerImpl
+ */
+ public static Debugger newInteractiveDebugger(InteractiveDebuggerRunner dr) {
+ return new InteractiveDebugger(dr, ExpressionCompilers$.MODULE$);
+ }
+
+ /**
+ * Factory method to get an instance of TraceDebugger
Review Comment:
I would avoid "instance of TraceDebugger", that isn't really true anymore
since there isn't a concept of a TraceDebugger, but it's also kindof intern
information. Maybe somethin glike
> Factory method to get a debugger that provides verbose trace output to a
PrintStream
Something simlar for the `@return`, it doesn't return a TraceDebuggerRunner,
it just returns a Debugger
##########
daffodil-core/src/main/java/org/apache/daffodil/api/DataProcessor.java:
##########
@@ -42,41 +36,8 @@
*/
public interface DataProcessor extends WithDiagnostics, Serializable {
/**
- * Obtain a new {@link DataProcessor} instance with debugging enabled or
disabled.
- * <p>
- * Before enabling, {@link DataProcessor#withDebugger} or {@link
DataProcessor#withDebuggerRunner} must be called to obtain
- * a {@link DataProcessor} with a non-null debugger.
- *
- * @param flag true to enable debugging, false to disabled
- * @return a new {@link DataProcessor} instance with debugging enabled or
disabled.
- */
- DataProcessor withDebugging(boolean flag);
-
- /**
- * Obtain a new {@link DataProcessor} with a specified debugger runner.
- *
- * @param dr debugger runner
- * @return a new {@link DataProcessor} with a specified debugger runner.
- */
- default DataProcessor withDebuggerRunner(DebuggerRunner dr) {
- Debugger dbg = null;
- InteractiveDebuggerRunner runner;
- if (dr instanceof TraceDebuggerRunner) {
- runner =
InteractiveDebuggerRunnerFactory.newTraceDebuggerRunner(System.out);
- } else if (dr != null) {
- runner = InteractiveDebuggerRunnerFactory.get(dr);
- } else {
- runner = null;
- }
-
- if (runner != null) {
- dbg = new InteractiveDebugger(runner, ExpressionCompilers$.MODULE$);
- }
- return withDebugger(dbg);
- }
-
- /**
- * Obtain a new {@link DataProcessor} with a specified debugger.
+ * Obtain a new {@link DataProcessor} with a specified custom debugger or
the provided
+ * TraceDebugger
Review Comment:
I would drop mention of `TraceDebugger`, just say Objection a new
DataProcesor with a specified debugger.
Maybe you can also add extra detail that mentions Daffodil.newTraceDebugger
or Daffodil.newInteractiveDebugger can be used to create daffodil provided
debugger to provide some options?
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/processors/DataProcessor.scala:
##########
@@ -163,12 +163,9 @@ class DataProcessor(
override def withDebugger(dbg: api.debugger.Debugger): DataProcessor = {
val optDbg = if (dbg eq null) None else Some(dbg)
- copy(areDebugging = optDbg.isDefined, optDebugger = optDbg)
- }
-
- def withDebugging(flag: Boolean): DataProcessor = {
- val newTunables = tunables.withTunable("allowExternalPathExpressions",
flag.toString)
- copy(areDebugging = flag, tunables = newTunables)
+ val newTunables =
+ tunables.withTunable("allowExternalPathExpressions",
optDbg.isDefined.toString)
Review Comment:
I think the previous logic was probably incorrect. If optDbg is not defined,
I would suggest we leave `allowExternalPathExpressions` as is and just use the
existing tunables as the newTunables. There might be some other use case were a
user manually enables this tunable but wants to disable debugging and we
shouldn't override that. NOte that we definitely do want to ensure this is
enabled if debugging is enabled since it's needed for evaluating debugger
expressions.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/debugger/InteractiveDebuggerRunner.java:
##########
@@ -20,30 +20,38 @@
import org.apache.daffodil.runtime1.debugger.InteractiveDebugger;
/**
- * Interactive Debugger Runner interface
+ * Interactive Debugger Runner interface to run interactive debuggers. Must be
implemented
+ * for custom debugger runners.
*/
-public interface InteractiveDebuggerRunner extends DebuggerRunner {
+public interface InteractiveDebuggerRunner {
/**
* initialize using an Interactive Debugger
*
* @param id interactive debugger
*/
void init(InteractiveDebugger id);
-
+
/**
- * @return interactive debugger command
+ * Called by Daffodil when there is a pause in parsing to determine what
+ * debugger actions should be taken.
+ *
+ * @return a debugger command that tells the Daffodil debugger what step to
+ * take next.
+ * @see <a target="_blank"
href='https://daffodil.apache.org/debugger/'>Daffodil Interactive Debugger</a>
- debugger commands
*/
String getCommand();
/**
- * output line
+ * Called by Daffodil when a debugger command has produce output. This method
Review Comment:
typo, should be "has produced"
##########
daffodil-core/src/test/scala/org/apache/daffodil/sexample/TestAPI.scala:
##########
@@ -211,13 +211,6 @@ class TestAPI {
val err = res.isError()
assertFalse(err)
- assertTrue(debugger.lines.size > 0)
- assertTrue(
- debugger.lines
-
.contains("-----------------------------------------------------------------
1\n")
- )
- assertTrue(debugger.getCommand().equals("trace"))
Review Comment:
I think this can be added back and information gotten from `customRunner`?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/DataProcessor.java:
##########
@@ -42,41 +36,8 @@
*/
public interface DataProcessor extends WithDiagnostics, Serializable {
/**
- * Obtain a new {@link DataProcessor} instance with debugging enabled or
disabled.
- * <p>
- * Before enabling, {@link DataProcessor#withDebugger} or {@link
DataProcessor#withDebuggerRunner} must be called to obtain
- * a {@link DataProcessor} with a non-null debugger.
- *
- * @param flag true to enable debugging, false to disabled
- * @return a new {@link DataProcessor} instance with debugging enabled or
disabled.
- */
- DataProcessor withDebugging(boolean flag);
-
- /**
- * Obtain a new {@link DataProcessor} with a specified debugger runner.
- *
- * @param dr debugger runner
- * @return a new {@link DataProcessor} with a specified debugger runner.
- */
- default DataProcessor withDebuggerRunner(DebuggerRunner dr) {
- Debugger dbg = null;
- InteractiveDebuggerRunner runner;
- if (dr instanceof TraceDebuggerRunner) {
- runner =
InteractiveDebuggerRunnerFactory.newTraceDebuggerRunner(System.out);
- } else if (dr != null) {
- runner = InteractiveDebuggerRunnerFactory.get(dr);
- } else {
- runner = null;
- }
-
- if (runner != null) {
- dbg = new InteractiveDebugger(runner, ExpressionCompilers$.MODULE$);
- }
- return withDebugger(dbg);
- }
-
- /**
- * Obtain a new {@link DataProcessor} with a specified debugger.
+ * Obtain a new {@link DataProcessor} with a specified custom debugger or
the provided
+ * TraceDebugger
*
* @param dbg debugger
Review Comment:
I think we need to say that debugger can be null and will remove any
debugging.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/debugger/InteractiveDebuggerRunner.java:
##########
@@ -20,30 +20,38 @@
import org.apache.daffodil.runtime1.debugger.InteractiveDebugger;
/**
- * Interactive Debugger Runner interface
+ * Interactive Debugger Runner interface to run interactive debuggers. Must be
implemented
Review Comment:
> Interactuve Debugger Runner interface to run the built-in Daffodil
interactive debugger.
I think "Must be implemented for custom debugger runners" is a bit
confusing, and almost implies custom debuggers must use this, but they don't.
Custom debuggers (like VS COde debugger) can implement the "Debugger" interface
and never have to use the InteractiveDebugger interface. This interface is only
for people that want to use the built-in debugger but want to control it with
custom input/output methods.
##########
daffodil-core/src/main/java/org/apache/daffodil/api/debugger/Debugger.java:
##########
@@ -20,7 +20,7 @@
import org.apache.daffodil.runtime1.events.EventHandler;
/**
- * debugger interface
+ * debugger interface that can get a bunch of events
Review Comment:
This is a bit too information. Maybe something like, "Debugger interface for
receiving debug events during Daffodil processing"
##########
daffodil-core/src/main/java/org/apache/daffodil/api/debugger/package-info.java:
##########
@@ -20,27 +20,21 @@
*
* <h2>Overview</h2>
* <p>
- * Daffodil comes with one prebuilt debugger, the {@link
org.apache.daffodil.api.debugger.TraceDebuggerRunner}, which outputs
+ * Daffodil comes with one prebuilt debugger, the TraceDebugger which is a
+ * {@link org.apache.daffodil.api.debugger.Debugger}, which outputs
* verbose information during the parsing processes, which can be used to aid
* in debugging a DFDL schema. For example, the {@link
- * org.apache.daffodil.api.debugger.TraceDebuggerRunner} can be used like so:
+ * org.apache.daffodil.api.debugger.Debugger} TraceDebugger can be used like
so:
*
* <pre>
* {@code
- * TraceDebuggerRunner tdr =
InteractiveDebuggerRunnerFactory.getTraceDebuggerRunner(System.out);
- * Daffodil.setDebugger(tdr);
+ * Debugger td = Daffodil.newTraceDebugger(System.out);
+ * dp.setDebugger(td);
* }</pre>
* <p>
- * Additionally, one may create their own debugger runner by implementing the
+ * Additionally, one may create their own debugger by implementing the
Review Comment:
I think we need to make it clear that there are kindof two different ways to
create a debugger, where this only really discusses one.
One way is to create a class that implements the Debugger interface.
The other way is to create a class that implements the
InteractiveDebuggerRunner interface, and call
Daffodil.withInteractiveDebugger(...) to get a Debugger.
Regardless of which way is chose, the Debugger can then be used by calling
Daffodil.withDebugger(...)
##########
daffodil-core/src/main/scala/org/apache/daffodil/runtime1/debugger/InteractiveDebugger.scala:
##########
@@ -52,10 +52,15 @@ import
org.apache.daffodil.runtime1.processors.unparsers.Unparser
case class DebuggerExitException() extends UnsuppressableException("Debugger
exit")
+/**
+ * Gets a bunch of events and uses runner to deal with events interactively
Review Comment:
Make this more format.
> Receives events from daffodil and uses the provided
InteractiveDebuggerRunner to determine how to handle those events
##########
daffodil-core/src/test/java/org/apache/daffodil/jexample/TestAPI.java:
##########
@@ -918,11 +919,11 @@ public void testAPI20() throws Exception {
parseXMLReader.setContentHandler(contentHandler);
parseXMLReader.setErrorHandler(errorHandler);
parseXMLReader.setProperty(DaffodilParseXMLReader.DAFFODIL_SAX_URN_BLOBDIRECTORY,
- Paths.get(System.getProperty("java.io.tmpdir")));
+ Paths.get(System.getProperty("java.io.tmpdir")));
Review Comment:
I think we still use 2-space indentation for .java files? Should we undo
these indentation changes?
##########
daffodil-core/src/main/java/org/apache/daffodil/api/debugger/package-info.java:
##########
@@ -20,27 +20,21 @@
*
* <h2>Overview</h2>
* <p>
- * Daffodil comes with one prebuilt debugger, the {@link
org.apache.daffodil.api.debugger.TraceDebuggerRunner}, which outputs
+ * Daffodil comes with one prebuilt debugger, the TraceDebugger which is a
+ * {@link org.apache.daffodil.api.debugger.Debugger}, which outputs
* verbose information during the parsing processes, which can be used to aid
* in debugging a DFDL schema. For example, the {@link
- * org.apache.daffodil.api.debugger.TraceDebuggerRunner} can be used like so:
+ * org.apache.daffodil.api.debugger.Debugger} TraceDebugger can be used like
so:
Review Comment:
change this to "trace debugger" to make it clear that a `TraceDebugger`
isn't an actual class/interface.
--
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]
