[rich] set colored_[repr/str] options in config to False

[pfackeldey]

[the following only affects logging using contrib.rich]

Hi @riga,

rich automatically adds (rather intelligent) coloring. Currently is clashes with law's own coloring in case str(task) or repr(task) is called and their coloring option is set to True. This leads to malformed output strings such as:

...[0;49;32mGroupCoffeaProcessesWithModel[0m([1;49;34mversion[0m=prod27, [1;49;34mnotify[0m=False, [1;49;34manalysis_choice[0m=bbww_sl, [1;49;34myear[0m=2017, [1;49;34mrecipe[0m=tth,...

It seems that by default it only affects str(task) in the following output:

[pid 60651] Worker Worker(salt=809229854, workers=1, host=..., username=..., pid=60651) running <str(task)>

(Seems that this here is the bad boy initially: https://github.com/spotify/luigi/blob/4d0576c7c265afcb228097af79f316ba0de0242c/luigi/worker.py#L157 ...should probably be %r instead of %s for self.task, what do you think?) [1] However just turning the coloring off safely prevents this malformed output always.

ps: It seems that (by default) this does not occur for the coloring setting of the targetsection, nevertheless I expect this to go wrong in the same way as for the tasks. Should these two setting be added in this PR too?

Best, Peter

[1]

In [0]: class A:
   ...:     def __str__(self):
   ...:         return "str"
   ...:     def __repr__(self):
   ...:         return "repr"
   ...:

In [1]: print("%s" % A())
str

In [2]: print("%r" % A())
repr

[riga]

Hi @pfackeldey , thanks for the PR!

Mh, I think in this case %s is preferable since one wants to see the human-readable string representation of the task, rather than the unambiguous one.

If I understand correctly, then this PR is a shortcut as in "when the rich handler is used, force certain config settings". I'm a bit hesitant with this approach as the colorization would also be disabled in user code where a task / target string is perhaps just print()'ed instead of logged. Wouldn't this be rather sth to put into the documentation of the rich contrib package?

[pfackeldey]

Yes this makes sense. I think if someone uses the rich package for printing/logging/... anyway they will use also rich's coloring directly. But I see your point. Forcing it is not a good idea. I could just move the "when the rich handler is used, certain config settings should be used" to the __doc__ string of this method (or even emit a warning?). What is your preferred way for handling this?

[riga]

(or even emit a warning?)

👍

The warning could be issued after the

logger.addHandler(rich_logging.RichHandler(level))

line in case one of the 4 color configs is True, using the info_once or warning_once method of the logger itself.

[pfackeldey]

Hi @riga ,

I hope this is now what you had in mind. I assumed the logger.warning_once somehow "caches" the log and only emits it once, so I did not add a break to the loop. Does that make sense to you?

[riga]

Yup, fine for me!