Skip to content
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

Question on the adequate behavior of formatting color. #57

Closed
needoptimism opened this issue Feb 17, 2019 · 7 comments
Closed

Question on the adequate behavior of formatting color. #57

needoptimism opened this issue Feb 17, 2019 · 7 comments
Labels
enhancement Improvement to an already existing feature

Comments

@needoptimism
Copy link
Contributor

Hello,

I have just found that the following code snippet,

from loguru import logger
config = {
    "handlers": [
        {"sink": sys.stderr,
         "level": "INFO",
         "format": "<Green>{time:YYYY-MM-DD HH:mm:ss.SSS}</Green> | <level>{level: <8}</level> | <cyan>{name}</cyan>:<cyan>{function}</cyan>:<cyan>{line}</cyan> - <level>{message}</level>"
         }
    ]
}
logger.configure(**config)
logger.info("Hello,")
logger.warning("World")

results in

<Green>2019-02-17 22:27:54.564</Green> | INFO     | __main__:<module>:16 - Hello,
<Green>2019-02-17 22:27:54.565</Green> | WARNING  | __main__:<module>:17 - World

It seems that tags(ex.<Green></Green>) which are not properly parsed by ansimarkup library are sent to sink without any blockers.

Is it the expected behavior when it was designed?
or should we raise an exception for this? :)

In my eyes, if color tags cannot be properly converted, it should give feedback to users instead of letting them recognize it after logs are recorded in sinks. :)
What do you think?

@Delgan
Copy link
Owner

Delgan commented Feb 17, 2019

This is by design of the ansimarkup library as stated by one example in the Readme:

# Unrecognized tags are left as-is.
parse("<b><element1></element1></b>")

I think it makes sense for Loguru too. I would like the logger to be as permissive as possible about accepted formats. If for some unknown reason an user wish to use xml tags within the logs format or set format="<{time}> <{function}> {message}", Loguru should not prevent him to do so.

In case of incorrect color like in your example, the raw tags appearing in the logs make it quite immediate to notice and easy to fix.

However, I agree, this may surprise the user as it's most of the time unintentional and hence an error. It would be more elegant to inform the user by raising an exception.

I think it would be possible to have the best of both worlds by adding a way to escape xml tags. So, the format stay permissive by allowing user to log raw tags if he wants, otherwise not escaped tags are parsed as color markups and raise an error if they are not recognized.


As we are about color markups, I have been thinking of replacing them with something else.

Basically, I don't like much mixing xml tags in the middle of Python formatting, I find it too verbose for hexadecimal colors like <#2DF12A>{message}</#2DF12A>, there is currently no way to escape them if user wish to log raw "<b>", and there is also others edge cases problems with opt(ansi=True).

I'm experimenting with something like:

class Color:

    def __init__(self, ansi):
        self.ansi = ansi

    def __format__(self, spec):
        return self.ansi + spec + "\x1b[0m"

color = {"green": Color("\033[92m"), "blue": Color("\033[94m")}
record = dict(message="Foo", level="DEBUG", color=color)

print("{color[green]:{level}} - {color[blue]:{message}}".format(**record))

I'm not convinced this looks much better, but at least it allows formatting to be fully handled by well-known Python format().

@needoptimism
Copy link
Contributor Author

Hello,
Thanks a lot for such a great explanation! I learned a lot! :D
When it comes to this issue, however, I do not have any cool idea at the moment.
I like your idea using a class, but I couldn't say it's nicer than the current permissive way.
Let me revisit this issue once I come across an elegant and nice idea.
Thanks for your help!

@Delgan
Copy link
Owner

Delgan commented Feb 18, 2019

Yep, this looks too much nested with the Color class. I'm not a big fan of my idea. 😁

I'm open to any good suggestion. Basically, we just need a way to escape the xml tags, or an alternative way to define colors in format.

@Delgan Delgan added the enhancement Improvement to an already existing feature label Mar 3, 2019
@Delgan
Copy link
Owner

Delgan commented Mar 4, 2019

I have been thinking to something like this:

format = "<green:{time}> - <level:{level}> - <blue:{message}>"

Color markups could be escaped by doubling the signs, like for {}: "<<green:{time}>>", otherwise if the name of the color is not recognized, an error could be raised.

@needoptimism
Copy link
Contributor Author

In my eyes, your new idea looks much better than the original one!! :) Not only the format gets shorter, but also is the better structure for raising exception. So, I would say this could give better impression to users in terms of interface. What do you think? :D

@Delgan
Copy link
Owner

Delgan commented Mar 5, 2019

Great, I'm pleased you like it. 😄

This feels somehow "weird" because this doesn't use a well-known markup language syntax. But overall, this reduces verbosity and allows explicit exceptions to be raised, so it's probably worth it.

@Delgan
Copy link
Owner

Delgan commented Jun 8, 2019

I modified the parsing of colors tags so it raises adequate errors if a tag name can't be parsed. I will be effective in the next v0.3.0 release. I had to re-implement the ansimarkup dependency directly into Loguru to apply those changes.

Contrary to what I suggested in my previous messages, I did not implement the "<red:message>" markup format for several reasons:

  • It's hard to parse and there is too much risk of conflict if the message contains > (would require escaping and be surprising)
  • There is no existing language markup looking like this, that would be a new custom language while XML/HTML tags style are well known
  • This would break compatibility with all configured loggers not using the default format

So, I stayed with the "<red>message</red>" markup style, but I made a few changes:

  • To reduce verbosity, I added a "</>" tag which can be used to automatically close the last opened tag, eg. "<red>message</>"
  • To escape a tag, one can prepend it with "\", eg. "\<red>" would not be parsed and would be displayed as "<red>" literally
  • If the tag name is not recognized, an exception is raised rather than silently ignoring the tag, eg. <Red>{message}</Red>" would raise a ValueError. To reduce risks of false-positive, tags are ignored if they contains spaces, eg. <Foo Bar> would not be parsed as a tag and would not raise error. The regex used to parse tag is </?((?:[fb]g\s)?[^<>\s]*)>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an already existing feature
Projects
None yet
Development

No branches or pull requests

2 participants