mbeckerle commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232536751
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -247,7 +247,7 @@ class DAPodil(
)
)
}
- case _ => session.terminate("failed to load sources")
+ case _ => session.abort(new ErrorEvents.SourceError("failed to load
sources"))
Review Comment:
Think about whether you need the string or not. It's already a
ErrorEvents.SourceError. Maybe the constructor for that doesn't need a string
argument. The ErrorEvents sub classes that are specific enough may not require
any further elaboration.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -367,7 +366,7 @@ class DAPodil(
s"couldn't find scopes for frame ${args.frameId}:
${data.stack.frames
.map(f => f.id -> f.stackFrame.name)}"
) *>
- session.terminate(s"couldn't find scopes for frame
${args.frameId}")
+ session.abort(new ErrorEvents.ScopeNotFoundError(s"couldn't
find scopes for frame ${args.frameId}"))
Review Comment:
Here you chose to make the log message different from the string passed to
the constructor. I'm not sure that's worth it. It's better to make just one
string, or no strings at all, and let session.abort do the logging as well to
reduce the clutter.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -67,9 +65,9 @@ object DAPSession {
def sendEvent(event: DebugEvent): IO[Unit] =
Logger[IO].info(show"<E $event") *>
IO.blocking(server.sendEvent(event))
- def terminate(message: String): IO[Unit] =
- sendEvent(new ErrorEvent(s"${message}. check debugger output for more
information.")) *>
- sendEvent(new Events.TerminatedEvent())
+ /** Send DebugEvent back to extension and exit session, ending debug */
+ def abort(event: DebugEvent): IO[Unit] =
Review Comment:
So by just changing this from terminate to abort are you saying all of these
usages of session.terminate were aborts indicating some unexpected failure?
If that's the case fine. One test is that the string in the message of an
abort would normally never be something a user would see, or need to read, or
would need internationalization, etc. They might see it as part of them filing
a bug report, but it's not normal for it to be displayed to a user.
##########
src/daffodil.ts:
##########
@@ -54,7 +54,14 @@ export interface BuildInfo {
scalaVersion: string
}
-export const errorEvent = 'daffodil.error'
+export const errorEvents = {
+ 'daffodil.error.unexpected': 'unexpected error',
Review Comment:
These are aborts, so really this doesn't need to be this pretty. Just
printing the original case class name coming over from the scala code would be
fine.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -212,8 +210,10 @@ class DAPodil(
debugee(request) match {
case Left(errors) =>
state.set(DAPodil.State.FailedToLaunch(request, errors, None)) *>
- Logger[IO].error(show"error parsing launch args:
${errors.mkString_(", ")}") *>
- session.terminate(show"error parsing launch args:
${errors.mkString_(", ")}")
+ Logger[IO].error(show"error parsing launch args:
${errors.mkString_("\n")}") *>
Review Comment:
Make session.abort do the logging also. That would condense this error
stuff to just one line.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/ErrorEvents.scala:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.debugger.dap
+
+import com.microsoft.java.debug.core.protocol.Events.DebugEvent
+
+/** Case classes for errors that we want to relay back to the extension */
+object ErrorEvents {
+ case class UnexpectedError(message: String) extends
DebugEvent("daffodil.error.unexpected")
Review Comment:
Since these are case classes, you can drop use of "new" before their names
in the code that constructs them.
That's optional in my book. Some people prefer to keep the "new", and I'm
fine with that.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/ErrorEvents.scala:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.debugger.dap
+
+import com.microsoft.java.debug.core.protocol.Events.DebugEvent
+
+/** Case classes for errors that we want to relay back to the extension */
+object ErrorEvents {
+ case class UnexpectedError(message: String) extends
DebugEvent("daffodil.error.unexpected")
+ case class UnhandledRequest(message: String) extends
DebugEvent("daffodil.error.requestunhandled")
+ case class LaunchArgsParseError(message: String) extends
DebugEvent("daffodil.error.launchargparse")
Review Comment:
DebugEvent really doesn't need this string arg. You can just do
```
lazy val event = Misc.getNameFromClass(this)
```
since you just need to display these strings as part of the abort info on
the client side. The class name will do just fine.
--
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]
