-
Notifications
You must be signed in to change notification settings - Fork 593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display a warning message for Beta and Exerimental tools. #4429
Conversation
if (warningMessage != null) { | ||
logger.warn(warningMessage); | ||
try { | ||
Thread.sleep(5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 seconds of sleep is going to drive me insane. .5 seems much more tolerable but still noticeable. We might want to add a command line option to disable the sleep and apply that to all the tests
Let's remove the sleep delay @cmnbroad. Red text + a border made out of exclamation points should be sufficient. |
The one problem with using color in the output is that it makes it gross to read logs that aren't on an interactive terminal that supports color. We might want to make an attempt to tell if we're connected to a terminal or an output file and remove the colors if so. |
I have this class from an ancient branch that makes dealing with colors nicer in some ways. It tries to avoid printing colors to non-interactive things and it makes it harder to forget a reset:
|
@lbergelson What do you think about putting the TerminalColors class in Barclay ? The same changes I'm making here need to be made in Picard. We could put a lot of this behavior into a shared CommandLineProgram base class that uses TerminalColors. |
@cmnbroad Barclay sounds like the best place for things that have to deal with the command line. Feel free to use that class or not. I'm not sure it's the greatest way to deal with colors, but it seems better than manually adding the escape codes everywhere. |
Codecov Report
@@ Coverage Diff @@
## master #4429 +/- ##
==============================================
+ Coverage 79.04% 79.119% +0.079%
- Complexity 16447 16521 +74
==============================================
Files 1047 1047
Lines 59189 59460 +271
Branches 9672 9747 +75
==============================================
+ Hits 46783 47044 +261
+ Misses 8644 8639 -5
- Partials 3762 3777 +15
|
Maybe https://github.com/fusesource/jansi to handle colors? |
/** | ||
* If a tool is either Experimental or Beta, log a warning against use in production a environment. | ||
*/ | ||
protected void printProductionWarning() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe warnAboutToolStatus()
would be clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* If a tool is either Experimental or Beta, log a warning against use in production a environment. | ||
*/ | ||
protected void printProductionWarning() { | ||
final String warningMessage = getProductionWarning(!(System.console() == null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this check isn't 100% reliable, I would take it out. Printing the literal codes in non-interactive use is not that big of a deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* @param useTerminalColor true if the message should include highlighting terminal colorization | ||
* @return a warning message if the tool is Beta or Experimental, otherwise null | ||
*/ | ||
protected String getProductionWarning(final boolean useTerminalColor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getToolStatusWarning()
might be clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 merge after addressing comments
Fixes #4367. Includes a sleep for 5 seconds; not sure thats a good idea, although the warning message scrolls off the screen pretty quickly, especially with Spark apps. This will need to be replicated in Picard as well.
Experimental:
Beta: