itamarst / eliot

Eliot: the logging system that tells you *why* it happened
https://eliot.readthedocs.io
Apache License 2.0
1.09k stars 65 forks source link

don't record / display exception reason if it's empty #479

Closed belm0 closed 2 years ago

belm0 commented 2 years ago

I'm not sure if it should be handled when the exception action is created, or by eliot-tree, but this seems wasteful for case of None or empty exception reason:

eliot-tree foo.json
38dbd553-f8a3-44b2-a950-b78cfd6a8d75
└── ...
    └── twist/5 ⇒ failed 2021-11-24 15:01:38Z
        ├── exception: Bar
        └── reason: 
itamarst commented 2 years ago

In general this is probably a waste of space... but not always. Consider an API that does raise KeyError(key). If the key is a blank string, that's still something you want to know when reading the logs, so you don't want the reason omitted in that case because it'd be confusing to the reader.

That being said, I wonder if eliot-tree should maybe use repr() on values? So you can tell difference between e.g. 12 or "12", or see if reason is a blank string. So you could file an issue there if you wish.

belm0 commented 2 years ago

I'm not sure I follow your example about KeyError. I don't think it's reasonable to be created with an empty string (as opposed to a string of one space, for the space key). (Anyway, you'd probably use a friendly reason like "mapping for key 'a' not found".)

For one-off exception class definitions internal to a codebase, it's common for str() to yield an empty string (specifically ''). Same for raising a built-in exception like ValueError without a message.

class Foo(Exception): pass

>>> str(Foo())
''

>>> str(ValueError())
''

If the author of the exception is not providing a str representation, it doesn't seem useful for eliot-tree to print a line. Absence of the line implies an empty str() result.