Skip to content

Commit

Permalink
Rework coloring function
Browse files Browse the repository at this point in the history
  • Loading branch information
@kpj
kpj committed Jul 31, 2015
1 parent 939cb99 commit eb9bda4
Showing 1 changed file with 9 additions and 9 deletions.
18 changes: 9 additions & 9 deletions lib/LaTeXML/Common/Error.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ use warnings;
use LaTeXML::Global;
##use LaTeXML::Common::Object;
use Time::HiRes;
use Term::ANSIColor qw(colored coloralias colorstrip);
use Switch;
use Term::ANSIColor qw(:constants);
use base qw(Exporter);
our @EXPORT = (
# Error Reporting
Expand All @@ -35,14 +36,13 @@ sub colorizeString {
my ($string, $alias) = @_;
return $string unless $COLORIZED_LOGGING;

my $out = colored($string, $alias);
return $out; }

coloralias('details', 'bold');
coloralias('success', 'green');
coloralias('info', 'blue');
coloralias('warning', 'yellow');
coloralias('error', 'red');
switch ($alias) {
case 'details' { return BOLD $string }
case 'success' { return GREEN $string }
case 'info' { return BLUE $string }
case 'warning' { return YELLOW $string }
case 'error' { return RED $string }
else { return $string } } }

#%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
# Note: The exported symbols should ultimately be exported as part
Expand Down

49 comments on commit eb9bda4

@brucemiller
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm... no, we don't want to introduce a dependence on Switch. Was there a problem with using coloralias?

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Perl simply doesn't have a "case" statement, and that's something we have to live with (there was a feature switch, but I think it was experimental and never got through to stable):
http://perldoc.perl.org/feature.html#The-%27switch%27-feature

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that switch statement can of course be replaced with a couple of ifs.

I think this made it easier to combine multiple features (like BOLD and RED).

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if-approach would look like this.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I almost forgot I know the answer of this already! Whenever you are dealing with equality to drive behaviour, you can see this as a form of multiple dispatch which Perl can support via... Hashes.

So you would do the behaviour declaration as a package-level var:

our %color_scheme = (
 details => BOLD,
 # ...
);

And then simply use it:

$color_scheme{$alias}

You can store all kinds of objects as hash values, including pointers to functions, so it's both a convenient and efficient "dispatch table". Much cleaner than the elsif chains.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I then use $color_scheme{$alias} directly at the print statement or still delegate its usage to the colorizeString function?

And how would I use this approach to simulate return BOLD RED $string;?

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is BOLD RED $string actually BOLD(RED($string)) ? If so, you can make an alias function:

sub bold_red {BOLD RED shift}

and store a reference to it in the hash:

key => \&bold_red

I think invoking it then becomes:

&{$color_scheme{$alias}}($string)

or something on those lines (maybe the & deref is optional?)

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, here is a free book on similar Higher Order Perl tricks if you're curious: http://hop.perl.plover.com/

Perl has all the infrastructure to do pretty neat functional programming, the significant drawback being that it isn't optimized for it and doing clever abstractions costs you heavily in runtime.

I am only sincerely advertising the hash-based dispatch in general, since that's heavily optimized and O(1).

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for the suggestion!

The problem with the dispatch table approach is that it seems to be impossible to combine both constants and function refs easily.
Consider

our %color_scheme = (
  ...
  error => RED,
  fatal => \&bold_red
);

and the call

return $color_scheme{$alias}->($string);

this works for function refs but throws

Can't use string ("") as a subroutine ref while "strict refs" in use at ...

for the constant RED.

These constants are defined as a list

my @colorlist = qw( ... RED ... )

in ... core_perl/Term/ANSIColor.pm.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks like you'll need an extra ref check to figure out if you are looking at a function or at a constant instead. Maybe it's easier to define a BOLDRED constant instead of making a function? That would make it consistent.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd recommend something like

my $spec = $color_scheme{$alias};
if (ref $spec) {
  return $spec->($string); }
else {
  return $spec($string); }

For some reason

return (ref $spec) ? $spec->($string) : $spec($string);

throws a syntax error.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you need &$spec($string)? But yes, that's what I meant.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, that was it!
But .. ehm .. why? Do tertiary operators require special magic?

Looks much nicer now :-)

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

But .. ehm .. why? Do tertiary operators require special magic?

Not sure... I am actually wondering if it would still work if you skip the ref check and just always do &$spec($string). Shouldn't that also work for the function args?

Btw, another curious link to check out - it's undecidable to figure out the abstract syntax tree of a Perl program before runtime:
http://www.perlmonks.org/?node_id=663393

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually yes :-)
The function now looks nice and cozy:

sub colorizeString {
  my ($string, $alias) = @_;
  return $COLORIZED_LOGGING ? &{$color_scheme{$alias}}($string) : $string; }

That link is super interesting!
That property is not specific to Perl, is it? It works for any language which allows ambiguous function signatures?

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the context sensitivity that makes it undecidable and most languages don't venture in that territory. In other words, all it takes to avoid this problem is to make it mandatory to provide function arguments, so that you always write foo() when you invoke the function foo, even when there are no arguments.

Because Perl tries to be super smart and lets authors be lazy with specifying arguments, you end up with these problematic cases.

@angerhang
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tips Deyan! Just did something similar to a snippet in sTeX.

BTW I was looking at switch as well, but gave it up, because of its fallacy plus no one else has (yet?) introduced it into sTeX or LaTeXML.

Getting ride of if and elsif alike : )

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this discussion it might be enough to simply check if the streams are ttys or not (and en-/disable colors accordingly).

A way of doing this in perl seems to described here.
What do you think?

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that would work.
Is it enough to only check STDERR, or does LaTeXML also do some STDOUT related stuff?

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love -t STDERR it's quite elegant.

But of course I have bad news :>

When the LaTeXML.pm flow "binds" all logging to a handle, it doesn't know in advance whether the passed logging information will be passed into a terminal, or into a file/server socket.

The decision is deferred to the executable - but in the current state of latexml, latexmlpost and latexmlmath, which don't use LaTeXML.pm, the solution you have in mind will work great.

However, for latexmlc, latexmls and ltxmojo, you will never pass on coloured output, as they bind the log in advance to a non-terminal filehandle. That's probably fine, since they almost never need coloring, but the case where latexmlc writes into the original STDERR will remain uncolored.

I am quite happy that -t enforces coloring only when there is 100% certainty there is a write to a terminal, which is quite safe for the exotic scenarios.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, given that there is theoretical desire to move towards latexmlc as a default executable, we should find a way to make it work.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that "non-terminal filehandle" have any special properties which could be detected?
If they behave similar to just piping into a file, there doesn't seem to be a way of detected it, right?

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I just talked to the guy who wrote latexmlc, apparently when it's really writing to STDERR it doesn't bind it. No problems at all.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only cases where we're binding STDERR away from the terminal is when the log is passed back via some channel, and in those cases it should never be colored anyway. Possibly accidentally good design, but good design nevertheless.

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am already using BOLD to distinguish "error" from "fatal" (both are red). Should I additionally underline "fatal" instead?

Also, this does probably also depend on the used font, terminal, etc. (in my terminal I can not see a difference between with and without BOLD due to my weird font setup :( )

Here's how it looks with bold everywhere.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using orange for error and red for Fatal in CorTeX, but that was also experimental. And not sure you can emulate orange here.

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make fun bits, like successful math parses, green :-)

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make fun bits, like successful math parses, green :-)

The Math parsing succeeded: part, or the individual elements?

And symbol "assumptions" should be yellow...

Again, the whole Symbols assumed as simple identifiers (with # of occurences):, or each individual symbol?

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this?
2015-08-11-210659_1119x210_scrot

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The yellow is somehow interesting. But I have seen it used for warnings, so Kim's screenshot looks pretty neat.

And indeed, it would be quite cool to have the final summary colored.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more comment, this time separate from this PR - while you're at it can you check whether there is an easy way to use coloring for the output of MakeMaker? I had checked some time back but couldn't find anything. I am quite used to colored test results from tool such as cmake or crate, would love to see that in LaTeXML.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering whether I should make the 'ltx:..' stuff colored as well.

Does the yellow look different for you?

easy way to use coloring for the output of MakeMaker?

Googling and looking through their repo don't show promising results. So I'd assume rather not :-(

But yes, cmake builds always look beautiful 😁

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ltx:XMath: and ltx:XMArg: also be colored, or only the corresponding numbers (0/1, 6/6)?

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 11, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brucemiller
Copy link
Owner

@brucemiller brucemiller commented on eb9bda4 Aug 12, 2015 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dginev
Copy link
Collaborator

@dginev dginev commented on eb9bda4 Aug 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy, happy Wednesday to everyone!

@kpj
Copy link
Contributor Author

@kpj kpj commented on eb9bda4 Aug 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Please sign in to comment.