You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "stevedlawrence (via GitHub)" <gi...@apache.org> on 2025/08/04 14:30:24 UTC
[PR] Remove Diagnostic.getMessageOnly API [daffodil]
stevedlawrence opened a new pull request, #1521:
URL: https://github.com/apache/daffodil/pull/1521
Daffodil 3.11.0 added the getMessageOnly API function to support a way to get just the message portion of a Diagnostic. This maintained the behavior of getMessage, which returned the message as well as the mode and schema context, but the intention was for a future release to drop getMessageOnly and change getMessage to return only the message.
The changes to the Daffodil API in commit c9a35ce2bd for Daffodil 4.0.0 modified getMessage to only return the message, so this removes getMessageOnly now that getMessage has that behavior.
DAFFODIL-2999
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2276655163
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
toString already contains this:
> Get a string containing the mode name, message, schema location, and data location combined into a single string
I also just pushed a fixup to clarify getMessage and recommend toString so it now says:
> Get the diagnostic mode name
>
> This does not include any additional details about the Diagnostic, such as
> mode name, schema context, or data location. For a human-readable
> representation of this diagnostic, including the message and all context
> information, use {@link #toString()}.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2276823969
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
Good point, I updated the internal Diagnostic getMessage and toString docs to provide correct documentation.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2251967963
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
I think without the context out how things used to work, it doesn't feel totally clear to me what the difference between `getMessage` and `getMessageOnly` would be. Like, from a new users perspective clearly `getMessageOnly` returns only the message, but then it's not clear what `getMessage` returns? If it also returns just the message (as it does now) that feels confusing having two functions that return the same thing. Or if we change `getMessage` back so it returns the message + mode + context, then that also feels confusing since it's returning more than just the message.
The approach taken here is "getMessage" returns just the message and "toString" returns mode + message + context. But maybe that's also not clear enough.
I think maybe the core issue is that we used to think of the "message" as mode + message + context, but some API users didn't, they wanted just the actual message without the mode and context. So "message" has kindof multiple meanings depending on who you talk to. So it feels our API wants to resolve that potential confusion. With 4.0.0 and other API changes, now feels like a good time to correct that since people will have to make changes regardless.
Maybe an alternative solution is `getMessage` returns just the message, and some new function returns message + mode + context (i.e. the same as toString()). Maybe `getMessageVerbose`, `getMessageDetailed`, or `getMessageWithContext? Or maybe `getMessage` returns the detailed message, and we have `getMessageBrief` or something return just the message without context?
I have no strong preference for what we pick, but in hindsight I'm not sure getMessage and getMessageOnly was the best choice.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "olabusayoT (via GitHub)" <gi...@apache.org>.
olabusayoT commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2252200644
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
I personally like the idea of getMessage returning only the message, and getMessageVerbose returning message+mode+context. I too got confused about why getMessage returned all those things
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2251761684
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
I recommend leaving this in. First the name is clearer about what it is returning. Second, why make people change it again?
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2276668575
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
Ah, I was looking at the toString definition in the Scala file, not the Java one. The one in the Scala file doesn't have those details, so that's why I was confused.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2276604630
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
Thinking more about this, and looking for other examples, consider `Throwable` as a reference. `Throwable.getMessage` returns just the message and `Throwable.toString` returns the message + context (i.e. class name). As another reference, I found the [javax.tools.Diagnostic](https://docs.oracle.com/javase/8/docs/api/javax/tools/Diagnostic.html) class that is also similar. That interface defines a `getMessage` function to get just the message, and then defines other functions to get other pieces of diagnostic context. It doesn't also define a function to get everything as a single string, because that's the purpose of the `toString` function.
So to be consistent with Java, I think we do really want `Diagnostics.getMessage` to return just the message portion and no additional context, and `Diagnostic.toString` can return the full mode + message + context as a string in human readable form.
These examples also feel like an argument against `getMessageVerbose`. In the java ecosystem, if you want a human readable version of a class and all its components (e.g. mode + message + context for our Diagnostic class), then that's what `toString` is for. That's well understood in Java, so adding a function that is essentially an alias for toString probably doesn't add much and could possibly cause more confusion (i.e. why call `getMessageVerbose` when I could just call `toString`?)
So after doing some research, I think I'm leaning towards the original PR as is. And we recommend that if you want just the message, use `getMessage`. If you want the entire diagnostic as a string, then use `.toString()`. It might require changes to API users, but it's a pretty simple suggestion for the migration doc, and makes our API more consistent with Java conventions.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2273470155
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
+1 to the idea of a "getMessageVerbose" or equivalent function.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "jadams-tresys (via GitHub)" <gi...@apache.org>.
jadams-tresys commented on code in PR #1521:
URL: https://github.com/apache/daffodil/pull/1521#discussion_r2276615091
##########
daffodil-core/src/main/scala/org/apache/daffodil/lib/iapi/Diagnostic.scala:
##########
@@ -128,8 +128,6 @@ abstract class Diagnostic protected (
*/
override def getMessage(): String = message
- override def getMessageOnly: String = message
Review Comment:
Honestly I forgot that was how the original PR was. That sounds good to me, my only suggestion is that we should add something to the documentation for toString mentioning that it has extra details and what they are.
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Remove Diagnostic.getMessageOnly API [daffodil]
Posted by "stevedlawrence (via GitHub)" <gi...@apache.org>.
stevedlawrence merged PR #1521:
URL: https://github.com/apache/daffodil/pull/1521
--
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: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org