Colorizing terminal output #224

ghost · 2018-02-15T02:40:02Z

These changes enabled messages printed to standard out to be colorized when they are either a warning or an error.

I've moved all print methods (that were previously in VM.java) into a new Output.java module. The module contains print, warning, and error methods that are called whenever SOMns prints something to standard output or standard error. Each method uses the static AnsiColor8 class to prefix their messages with the corresponding ANSI color code: red for errors, yellow for warnings, and no coloring for regular messages.

To provide access to the printing methods from Newspeak, I've added a pair of new System primitives: PrintWarningPrim and PrintErrorPim. These can be invoked, via the System module.

Finally, the ANSI coloring is turned off by default (since it's not supported by Eclipse's Console widget). The argument --ansi-coloring, or the shorter version -ac, can be added when invoking SOMns from the command line to turn the ANSI coloring on.

smarr

Thanks, I like it pretty :)

Could you please have a look at the comments?
I am also wondering whether this needs to have a flag to disable it, some java property.
When we might run in an environment without ansi term support, this might cause issues.
I checked whether that can be automatically detected, but that seems to be rather problematic.

smarr · 2018-02-15T06:44:05Z

core-lib/System.ns

@@ -25,7 +25,8 @@ class System usingVmMirror: vmMirror = Value (
  (* Exiting *)
  public exit: error  = ( vmMirror exit: error )
  public exit         = ( self exit: 0 )
-  public error: msg   = ( self printNewline: 'ERROR: ' + msg. self exit: 1 )
+  public warning: msg   = ( vmMirror printWarning: msg. )


one space too much for alignment before =?

smarr · 2018-02-15T06:45:54Z

src/som/VM.java

@@ -315,17 +315,43 @@ public void errorExit(final String message) {
    requestExit(1);
  }

+  private static class AnsiColor8 {
+    private static String black   = "\u001b[30m";


Is there a specific reason why these are not final?
and, if they should be final, could we capitalize the names, e.g., BLACK to indicate them as constants?

It's also getting unwieldily here.
Could you move all printing into something like an Output or Terminal class?
The som package is probably the right place for it.

ghost · 2018-02-15T22:45:04Z

Hi Stefan,

I've updated the changes to include a new argument, added to the ./som Python script and later VMSettings (I'm happy to change to a Java property if you like, but I thought this was a reasonable solution). When this flag is included, the ANSI coloring is disabled. I didn't know how to name the flag, so currently I've used -O for the short name and --no-ansi for the long name.

I've also changed the fields of the AnsiColor8 class to be public and finalized and refactored the print, warning, and error methods so that other colors can be used.

smarr · 2018-02-15T22:51:03Z

som

@@ -105,7 +105,8 @@ parser.add_argument('-T', '--no-trace', help='do not print truffle compilation i
 parser.add_argument('--no-graph-pe', help='disable Graph PE',
                    dest='graph_pe', action='store_false', default=True)

-
+parser.add_argument('-O', '--no-ansi', help='Turns off ANSI coloring for output from interperter',
+                    dest='no_ansi_coloring', action='store_true', default=False)


Please avoid negation where possible.
So, dest='ansi_coloring', action='store_false', default=True. This is much more explicit and easier to read.

smarr · 2018-02-15T22:52:06Z

som

@@ -224,6 +228,8 @@ if not args.interpreter and args.igv_to_file:
    flags += ['-Dgraal.PrintIdealGraphFile=true']
 if args.igv_methods:
    flags += ['-Dsom.igvDumpAfterParsing=true']
+if args.no_ansi_coloring:


Why are there two flags added?
-Dsom.noAnsiColoring=true and -Dsom.useANSIColor=false?

Haha, missed this during clean-up.

smarr · 2018-02-15T22:52:32Z

src/som/Output.java

+import som.vm.VmSettings;
+
+
+public class Output {


Please add a brief comment.

smarr · 2018-02-15T22:53:01Z

src/som/Output.java

+  /**
+   * This method is used to print reports about the number of created artifacts.
+   * For example actors, messages and promises.
+   */


Please first the comment, then the annotation.

https://github.com/smarr/SOMns/blob/master/src/som/VM.java#L352

Yeah, there should be a linter rule for that...

smarr · 2018-02-15T22:55:53Z

src/som/Output.java

+  public static void print(final String msg, final String color) {
+    // Checkstyle: stop
+    if (VmSettings.NO_ANSI_COLOR_IN_OUTPUT) {
+      System.out.print(msg);


This isn't a correct patch.
We used to output to System.err when showing errors.

smarr · 2018-02-15T22:57:34Z

src/som/vm/VmSettings.java

@@ -29,6 +29,8 @@

  public static final boolean IGV_DUMP_AFTER_PARSING;

+  public static final boolean NO_ANSI_COLOR_IN_OUTPUT;


No negation, please!

smarr · 2018-02-15T23:00:28Z

And, I can't see the logs currently because of Travis issues, but I am pretty sure they are failing.
We got tests for a reason :)

ghost · 2018-02-15T23:00:32Z

Hmm, it seems the Eclipse Console has ANSI support now (or perhaps I turned it off somewhere). Maybe the flag should be to turn the coloring on rather than off?

smarr · 2018-02-15T23:02:04Z

Hmm, it seems the Eclipse Console has ANSI support now (or perhaps I turned it off somewhere). > Maybe the flag should be to turn the coloring on rather than off?

Say what? I don't understand, but this doesn't make sense to me.
Why would you disable it by default if the Eclipse console has support for it?

ghost · 2018-02-19T22:27:17Z

Why would you disable it by default if the Eclipse console has support for it?

Sorry, I had meant to say that Eclipse console does not have support for it.

- move all output related code into Output class - colorize print, warning, and error methods - output errors to stderr - add som.useAnsiColoring flag to control use of colors - in launcher, determine use of colors based on whether it looks like a terminal Co-authored-by: Stefan Marr <[email protected]> Signed-off-by: Stefan Marr <[email protected]>

smarr · 2018-02-22T09:12:25Z

Since this is a conceptually pretty simple change, I did condense the history.

smarr requested changes Feb 15, 2018

View reviewed changes

Repository owner deleted a comment Feb 15, 2018

smarr requested changes Feb 15, 2018

View reviewed changes

smarr added the enhancement Improves the implementation with something noteworthy label Feb 20, 2018

smarr added this to the v0.6.0 - Black Diamonds milestone Feb 20, 2018

smarr approved these changes Feb 22, 2018

View reviewed changes

smarr merged commit de36c88 into smarr:dev Feb 22, 2018

smarr changed the title ~~Colorizing standard output~~ Colorizing terminal output Feb 22, 2018

ghost deleted the somdev branch March 13, 2018 23:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colorizing terminal output #224

Colorizing terminal output #224

ghost commented Feb 15, 2018 •

edited by ghost

Loading

smarr left a comment

smarr Feb 15, 2018

smarr Feb 15, 2018

smarr Feb 15, 2018

ghost commented Feb 15, 2018

smarr Feb 15, 2018

smarr Feb 15, 2018

ghost Feb 15, 2018

smarr Feb 15, 2018

smarr Feb 15, 2018

ghost Feb 15, 2018

smarr Feb 15, 2018

smarr Feb 15, 2018

smarr Feb 15, 2018

smarr commented Feb 15, 2018

ghost commented Feb 15, 2018

smarr commented Feb 15, 2018

ghost commented Feb 19, 2018

smarr commented Feb 22, 2018

		@@ -29,6 +29,8 @@

		public static final boolean IGV_DUMP_AFTER_PARSING;

		public static final boolean NO_ANSI_COLOR_IN_OUTPUT;

Colorizing terminal output #224

Colorizing terminal output #224

Conversation

ghost commented Feb 15, 2018 • edited by ghost Loading

smarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarr commented Feb 15, 2018

ghost commented Feb 15, 2018

smarr commented Feb 15, 2018

ghost commented Feb 19, 2018

smarr commented Feb 22, 2018

ghost commented Feb 15, 2018 •

edited by ghost

Loading