#623: suppress logging null/empty lines

[JMFaust]

Summary
Suppress logging blank/null lines, which eliminates the blank error line that can sometimes appear as reported in issue #623 .

You may wish to move the if check for null/empty strings to the top of the function to suppress false info messages as well, but I leave that to you.

Tests and Documentation
I ran the modified version across multiple builds locally and all false errors disappeared, while allowing valid warnings/errors to be reported as before.

[JMFaust]

Note: this issue doesn't specifically affect the maven build, but build tools such as Jenkins look through the maven build log for "[ERROR]" and will incorrectly treat the build as failing due to this false flag.

[rubensa]

Please @eirslett

Merge this change as I'm facing false ERROR logs in Maven running a 'npm install' on Ubuntu 17.10 with Node v8.9.4 (no errors running on command line).

[hhsde]

@eirslett , any thoughts on when/why this change can/cannot be merged?

[eirslett]

This isn't really a problem with the Maven plugin. It's a problem with npm, which logs to stderr, when it should have been logging to stdout. And it's a problem with Jenkins, which should determine build status based on exit code, not on contents of the build log.
I'm reluctant to merge this fix, because there may be other, legitimate reasons to log empty lines in the build output.

[JMFaust]

Since the change just flags an empty log as passing. I'm not sure under which condition that an empty log would be an error if no exception or error was provided.

[eirslett]

There's a number of reasons why you would want to log empty lines, especially readability-wise. If you need to log an error and display an ascii diagram for example, or separate paragraphs of text in a lengthy error report?

[pitgrap]

We see a lot of empty error lines too, since we switched to yarn/babel/webpack. That's not breaking our Jenkins builds, but it doesn't help anything either. Maybe we could make a compromise to not supress empty lines, but instead logging them to info instead of error. What do you think?

[eirslett]

I'm still reluctant, I don't think it' this plugin's responsibility to supporess empty lines. Maybe it should be fixed in yarn instead? You could argue that they shouldn't be logging empty stderr lines in the first place. If you build is set up fail fail if it finds the string "Error" in the log, then it would still appear if you ran yarn/babel/webpack, I suppose? Anyways, it's not good practice to fail a build on conditions like that, the proper way is to use process exit codes. Exit code 0 = success, all others = failure.

[pitgrap]

I totally agree with you and your arguments, but maybe I can give you an example, why it would be helpful if this plugin is not logging (false) errors:
We're shipping a frontend workspace with this plugin to our customers. Everything works fine. But some customers are looking into the logs and we get a lot of support requests like "there are errors in the log" and we have to explain them everytime, that's not a problem, its a problem of yarn/npm etc. and they should ignore it...
This is why I added a small check for warnings in npm and would like to add the same for yarn (see #713). And if we could avoid empty error lines too, nothing changes, some more people are just happy.

[pitgrap]

Hey @eirslett, could you please take a look at the Pull-Request #721? I did not remove the empty lines, but log them at least as info instead of error. Is that a reasonable compromise to you?

[s50600822]

I think neither node or java is wrong, rather, there are mismatches in the int ranges, that presents certain logging levels in each language(and possible logging libs in each). In my opinion, trying to patch it this way won't scale.
If you find filtering a particular case helps your particular workflow then you can apply it in your own build (filtering the build output for example) - or fix the node lib/process to not produce logging with level(the integer presentation) contradict that of Java logging.