Replace uses of Trace.Trace* with new wrapper methods #3118
Conversation
…erMessage.AsyncWrite - a bare call to TraceWarning looks to much like debug code. In instances where Trace.TraceWarning is called with a string literal (rather than a string resource) I have left it alone. Whether or not these should remain in the code is a matter for review.
|
@nickshulman @brendanx67 any thoughts on this? |
|
I was hoping that we would eventually get to a place where the Skyline code base had lots of logging statement and, when Skyline is behaving mysteriously, users would be able to go to "Tools > Options" and increase the verbosity level and maybe see some helpful information. |
|
Having a UserMessage class seems like a good start so that we are clearer
about what we want the user to see. The problem with the old situation was
that it looked like debug code and was more likely to escape notice that
the messages were not localizable, and also increased the likelihood of
leaving debug code in place thinking it was meant for the user to see.
…On Thu, Aug 29, 2024 at 8:15 AM nickshulman ***@***.***> wrote:
I was hoping that we would eventually get to a place where the Skyline
code base had lots of logging statement and, when Skyline is behaving
mysteriously, users would be able to go to "Tools > Options" and increase
the verbosity level and maybe see some helpful information.
Currently, we effectively have that verbosity level set at "Warning" and
therefore all Warning and Fatal messages get written to the Immediate
Window.
How can we get there from here?
—
Reply to this email directly, view it on GitHub
<#3118 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACYBWUHOFE3KASEGVEQRFJDZT43IBAVCNFSM6AAAAABMM6VGM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJYGA2DQMRRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
So are we satisfied with this as a step in the right direction? And with leaving the instances where Trace.TraceWarning is called with a string literal (rather than a string resource) as-is, being debug code? I'd like to get it merged and off my PR list. |
|
@brendanx67 are we satisfied with this as a step in the right direction? And with leaving the instances where Trace.TraceWarning is called with a string literal (rather than a string resource) as-is, being debug code? I'd like to get it merged and off my PR list. |
|
Not very satisfied, since all Trace.TraceWarning() output goes to the Immediate Window. I would like to see a solution that now uses a different mechanism, if we are going to leave these in unlocalized for debugging purposes. I don't mind adding another class called DebugMessage which we use to wrap Trace.TraceWarning() for debug use, and put the call to Trace.TraceWarning() in a conditional for being under a debugger, which prevents a user from ever seeing the message. Either that, or we need to create a different mechanism for UserMessage adding messages to the Immediate Window and disconnect this "clever" overloading of Trace.TraceWarning(). |
|
Sorry you have ended up saddled with this for being the person I finally noticed using this feature that Nick added without my realizing the long-term impact. |
|
What is the Skyline "Immediate Window" (I think of Visual Studio when I read that)? |
|
Yeah. It is a bit like the VS Immediate Window. I had Dany Broudy create it for external tools as a place to write external tool console output that could be copied and pasted, and we also added a bit of ability to run commands to the window. I can't remember exactly what, but definitely the ability to run a tool from the Immediate Window. |
|
I think I'd have more clarity on this if I knew what @nickshulman thinks should happen with those untranslated messages. |
I think any TraceWarning's that we wouldn't want the user to see should be changed to TraceInformation. |
…aceInformation so users won't see them Add a comment to TraceWarningListener about how to temporarily enable Information level messages on Console/ImmediateWindow Add custom code inspections to check for proper use of Trace in Skyline code
Done, but my gut (and Brendan's too, I think) is that we want to wrap that in something that makes the intent clear e.g. "AnsyncDebugMessage" while hiding the actual implementation (TraceInformation). |
|
Let’s create a parallel class to UserMessage called DebugMessage and wrap
this use in it and then add to our static analysis that these are the only
two cases of these functions in the code. So, that misuse doesn’t creep
back.
…On Mon, Sep 23, 2024 at 10:53 AM Brian Pratt ***@***.***> wrote:
I think any TraceWarning's that we wouldn't want the user to see should be
changed to TraceInformation.
Done, but my gut (and Brendan's too, I think) is that we want to wrap that
in something that makes the intent clear e.g. "AnsyncDebugMessage" while
hiding the actual implementation (TraceInformation).
—
Reply to this email directly, view it on GitHub
<#3118 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACYBWUDTJQMKEMXLVQDRNNTZYBIRDAVCNFSM6AAAAABMM6VGM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRYHE3TQOBYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I think it would make more sense to have a single class called "Messages" instead of two separate classes "UserMessage" and "DebugMessage". |
|
Sure. That also is fine with me.
…On Mon, Sep 23, 2024 at 3:33 PM nickshulman ***@***.***> wrote:
Let’s create a parallel class to UserMessage called DebugMessage and wrap
this use in it and then add to our static analysis that these are the only
two cases of these functions in the code. So, that misuse doesn’t creep
back.
I think it would make more sense to have a single class called "Messages"
instead of two separate classes "UserMessage" and "DebugMessage".
Could we put this thing into CommonUtil so that it can be used by
ProteomeDb and Common? (But, then there's the issue that AutoQC and Skyline
Batch don't currently do anything with these messages).
—
Reply to this email directly, view it on GitHub
<#3118 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACYBWUFXDWQGBY5I4S7F5H3ZYCJMDAVCNFSM6AAAAABMM6VGM2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRZGY2DONZSGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…y introduced DebugMessage.AsyncWrite
…m/ProteoWizard/pwiz into Skyline/work/20240812_UserMessage
|
So, Messages.WriteAsyncUserMessage and Messages.WriteAsyncDebugMessage? |
…led in a single class
bspratt
changed the title
The thinking is that a bare call to TraceWarning looks too much like debug code.
In instances where Trace.TraceWarning is called with a string literal (rather than a string resource) I have left it alone as this does in fact seem more like debug code. Whether or not these should remain in the code is a matter for review.