Stop using the `logger.error("{}", str(exc))` idiom

[portante]

When we have a simple string to log, why not just log it?

For example, instead of self.logger.error("{}", str(exc)), just use self.logger.error(str(exc))?

Originally posted by @portante in #3091 (comment)

[dbutenhof]

If the string includes {} we'll get logger failures because there's no parameter provided for the format directive. Doing "{}", arg means we're protected from the characters in arg.

That's not a hypothetical; I started doing it after it tripped me up a couple of times.

[portante]

How will str(exc) include formatting characters?

[webbnh]

Well, if one of the arguments happens to be an empty set or dictionary....

"You silly git, you framboised the value: the remaining keys are {}"

[portante]

I am not following. Can you give an example that actually fails using self.logger.error(str(exc))? Meaning, either we have an exception message that when formatted using str() leaving dangling format characters (which it shouldn't) or some standard library or dependent library does. That seems weird.

[webbnh]

I can't give you a concrete example of real code off the top of my head, but I remember when Dave hit one.

However, as a hypothetical, imagine that we have an exception object like this:

class MyException(Exception):
    def __init__(self, things: set):
        self.message = f"You broke the things:  {things}"

    def __str__(self):
        return self.message

If the code does a raise MyException({}), then, when it is caught, str(exc) will yield "You broke the things: {}". If this string is then passed as the first argument to logger.error(), it will interpret the {} as a format specifier instead of as the literal representation of an empty set (or dict).

As Dave said, this situation is easily finessed by calling logger.error("{}", str(exc)), which removes the ambiguity.

[portante]

If that was code under our control, shouldn't that be written where the formatting is done in the __str__() method:

def __init__(self, things: set):
    self._things = things

def __str__(self):
    return f"You broke things: {self._things}"

If there are libraries we depend on, or the standard library does this, then that is another thing.

Also, `self.logger.error("{}", exc) work, too, and avoids the string formatting when the log message won't be emitted.

[webbnh]

shouldn't that be written [...]

Yeah...and, it probably was. But, that doesn't have anything to do with whether we need the self.logger.error("{}", exc) idiom instead of just using self.logger.error(str(exc)).

The point was that it wasn't unreasonable for the string to end up with a {} in it, which meant that it couldn't be used directly as the log message argument.

[portante]

I personally see it to be very reasonable to expect exceptions to format without braces in the resulting string. But, I'd be willing to compromise with just seeing, self.logger.error("{}", exc). I don't think we should be doing both the empty format string and the str(exc).

[webbnh]

I personally see it to be very reasonable to expect exceptions to format without braces in the resulting string.

I'm in at least 90% agreement with that...but, I don't want to be bitten in the rare case where having a {} in the resultant string would make sense but it causes something to break. And, it strikes me that incurring the cost of formatting the string on an error path is more than worthwhile to avoid that corner case.

I'd be willing to compromise with just seeing, self.logger.error("{}", exc). I don't think we should be doing both the empty format string and the str(exc).

I think that the "{}" formatting directive implies a string input, and I think that Python will coerce the argument into a string for that purpose, and I expect that it will do so by calling str() on the argument (at least, if it's not already a string)...so, on that basis the explicit invocation of str() should be superfluous. I guess that Dave and I would put it there out of a sense of uncertainty or even paranoia, since it shouldn't hurt anything and it might not even have a measurable effect (but, if it is needed, then we're covered). 🥴

[dbutenhof]

I'm not entirely certain anymore, but it's possible that our recent convention of using str(exception) came into play as a result of Webb's observation of the "cute" behavior of str.__repr__ of inserting implicit quotes.

That is {exception!r} ends up with something like Exception('message') vs {str(exception)!r} returning just "message", which is generally what one wants except when debugging. Using str or repr explicitly resolves the ambiguity, which can sometimes be enough reason.