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

Option to not raise on unknown color tags #197

Closed
gazpachoking opened this issue Jan 1, 2020 · 6 comments
Closed

Option to not raise on unknown color tags #197

gazpachoking opened this issue Jan 1, 2020 · 6 comments
Labels
enhancement Improvement to an already existing feature

Comments

@gazpachoking
Copy link
Contributor

I think it would be a nicer behavior to not raisie on unknown tags (or an option that could turn this off.) We ran into logging crashes with the first log lines we tried to color, because some reprs that ended up in the message were with angle brackets. The angle brackets were via a variable in the message, so escaping ends up being a bit cumbersome. logger.opt(ansi=True).info('<green>colored message</> other stuff {here}', here=dobracketescaping(somevar)) I see this used to be the behavior, but got changed to help people get the color tags right in #57 .

If you don't want to change this behavior back, maybe include a function, or formatter that does the escaping? I see in the docs there is a regex if the user wants to do it, but it might be nice to include. Potential methods:

logger.opt(ansi=True).info('<green>color</> {}', logger.ansiescape(somevar))
logger.opt(ansi=True).info('<green>color</> {:ansiescape}', somevar)
@Delgan
Copy link
Owner

Delgan commented Jan 1, 2020

Hum... Actually, wouldn't the most desirable default behavior be to totally ignore tags present in additional arguments?

In logger.info(message, somevar=somevalue) the message argument is the "static" part, the developer can easily configure color tags manually. It's better to raise an error if he got it wrong somehow. However, the somevalue is expected to be "dynamic", it can theoretically take any form and is probably not expected to contain any color tag. It makes little sense to include the representation of somevalue in the parsing procedure. If one would like to include color tags present in somevalue, it can just use f-string anyway, so it's not much a problem.

For now I'm ignoring the technical feasibility of such solution, I'm just looking for the most convenient / less surprising behavior. Current approach seems like being completely wrong. 😕

@gazpachoking
Copy link
Contributor Author

Hum... Actually, wouldn't the most desirable default behavior be to totally ignore tags present in additional arguments?

Actually, that sounds great. I hadn't thought of it.

@Delgan Delgan added the enhancement Improvement to an already existing feature label Jan 1, 2020
@gazpachoking
Copy link
Contributor Author

gazpachoking commented Jan 1, 2020

This could be implemented by using a custom Formatter class when the ansi option was on. This would prevent it from running on unused arguments, as well as prevent it from running whenever ansi option wasn't on. Something like:

class MarkupEscapingFormatter(string.Formatter):
    def format_field(self, value, format_spec):
        value = super().format_field(value, format_spec)
        if isinstance(value, str):
            value = re.sub(r"(\\?</?((?:[fb]g\s)?[^<>\s]*)>)", r"\\\1", value)
        return value

@Delgan
Copy link
Owner

Delgan commented Jan 1, 2020

I think it's possible to implement a workaround without explicitly escaping arguments to be formatted. Basically, I will need to use string.Formatter().parse() to pre-colorize the message while ignoring formatting tokens. Actually, something very similar is already done when ansi=True to ensure color tags are properly handled "as a whole" between format and message:

loguru/loguru/_handler.py

Lines 242 to 268 in d4851c4

@staticmethod
@functools.lru_cache(maxsize=32)
def _memoize_ansi_messages(format_, ansi_code, message):
markups = {"level": ansi_code, "lvl": ansi_code}
ansimarkup = AnsiMarkup(custom_markups=markups, strip=False)
def parse(string_, *, recursive=True):
for text, name, spec, _ in string.Formatter().parse(string_):
ansimarkup.feed(text, strict=False)
if spec and recursive:
yield from parse(spec, recursive=False)
if name and name[:8] in ("message", "message.", "message["):
yield ansimarkup.feed(message, strict=True)
messages = list(parse(format_))
class AnsiDict:
def __init__(self, record):
self._record = record
self._messages = iter(messages)
def __getitem__(self, key):
if key == "message":
return next(self._messages)
return self._record[key]
return AnsiDict

It's a bit tricky. Give me a few days to get my head around this. 😄

@gazpachoking
Copy link
Contributor Author

I think it's possible to implement a workaround without explicitly escaping arguments to be formatted.

Oh interesting, nice. I had a peek to see if I could send over a PR, but it will probably take me longer to get my head around what's already going on there than for you to come up with a solution.

Delgan added a commit that referenced this issue Jan 19, 2020
First, ansi markup in "*args" and "**kwargs" are ignored when used while
"opt(ansi=True)". This caused errors, as such arguments can contains
tags which are not intended to be used as color markups.

Secondly, it strips the color markups present in the "record[message]"
so it can be used somewhere else where the "ansi" context is lost (like
when logs are emitted through socket). Actually, the colors are part of
the handler so it makes sense to strip them from the record.

These change required a whole refactoring of the coloration process. It
has been implemented with optimization in mind: trying to parse the ansi
message only once, then using the generated tokens to colorize the
message appropriately in each handler. It could certainly be improved,
though.
@Delgan
Copy link
Owner

Delgan commented Jan 19, 2020

Ok, I just released v0.4.1 which should fix that. 😉

It has been quite a headache to refactor color handling between format, message and arguments while also optimizing it for performances. Fortunately, I think I ended up with something quite correct, although the code is rather complicated.

Also, note that I deprecated .opt(ansi=True) in favor of .opt(colors=True). Using "ansi" always felt wrong to me, as this is actually an implementation detail. It was not a very descriptive parameter for beginners.

Anyway, I think it will be much more convenient now that the arguments are no longer taken into account for coloring. It should have been like this from the beginning.

@Delgan Delgan closed this as completed Jan 19, 2020
@Delgan Delgan mentioned this issue Jul 7, 2022
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