Header output #29663
Header output #29663
Conversation
|
In the screenshot, the console on the left contains test results before the proposed changes and the console on the right contains test results after the proposed changes. Did you intend to lose the color in |
|
Yeah, I had to drop the color via the input file in order to make the regexp work in the test. The difference added here is the "Moose Test App" header and it's prefixes. |
|
Job Coverage, step Generate coverage on 9234e1f wanted to post the following: Framework coverage
Modules coverageCoverage did not change Full coverage reportsReports
This comment will be updated on new commits. |
||||||||||||||||||||||||||
|
Job Documentation, step Docs: sync website on 9234e1f wanted to post the following: View the site here This comment will be updated on new commits. |
| if (hdr.length() != 0) | ||
| { | ||
| if (multiAppLevel() > 0) | ||
| MooseUtils::indentMessage(_name, hdr); |
There was a problem hiding this comment.
I was confused as to why this line wasn't incorporating the app name into the message. So I investigated and it turns out that _name = "". So I investigated more and found a bug. Could you please pull #29671 and see if the headers behave as you want with the fix?
There was a problem hiding this comment.
Yup! That took care of that problem, thanks! Is there a reason for all the complications with indentMessage? Seems like _console takes care of it altogether.
There was a problem hiding this comment.
I guess the difference is slight, for example this is your newest fix:
There was a problem hiding this comment.
And using _console tracks that extra new line:
There was a problem hiding this comment.
This is because
std::string
MooseTestApp::header() const
{
return std::string("MOOSE Test App\n");
}
There was a problem hiding this comment.
Yes, I get that, and I get that the fix in #29671 is good no matter what. I suppose what I'm getting at is Moose::out the correct output stream for headers, or is _console the right way. Certainly _console is much cleaner in the code, and captures all of the header, including whitespace.
There was a problem hiding this comment.
I think it comes down to if you want the header in
_consoleorMoose::out?
After speaking with @permcody, it seems our official stance is to prefer _console over Moose::out. I would be fine with your changes, just re-add the check if (hdr.length() != 0) so we don't print empty lines.
Also, I do think that polluting all console outputs with a bunch of "MOOSE Test App" is not worth having the test for these changes, so I would also get rid of the virtual header method in MooseTestApp and drop the test.
As a final caveat, I would also consider how setting Outputs/console=false would affect the output as opposed to using Moose::out and if we are willing to accept that difference (if it exists).
So in conclusion, if you want to change Moose::out to _console, I'm fine with that. If you don't want to bother and are happy with my existing fix, I'm also fine with that. Just let me know.
There was a problem hiding this comment.
seems our official stance is to prefer
Yeah, that's what I seemed to remember...
not worth having the test
I suppose so...but my next PR is going to suppress headers all together via a command line option #29669. It seems somewhat anti-moose to not have tests...but up to you all.
I would also consider how setting Outputs/console=false would affect the output as opposed to using Moose::out
Outputs/console=false with _console and Moose::out suppresses output from the parent app, but not the header. I don't see a difference. Do you anticipate one?
There was a problem hiding this comment.
Do you anticipate one?
I didn't know what to anticipate, it was just something to check.
There was a problem hiding this comment.
I suppose so...but my next PR is going to suppress headers all together via a command line option #29669
That one will definitely need test(s). If another reviewer tells you that you need tests related to something like this, please tag me, as I just got review authority and am still learning.
|
I'm just going to close this and use |
Changes header output to
_consolestream. In order to test this, I needed an app to actually have a header out output. I added one to theMooseTestApp, hopefully that is okay.ref #29662