logging error message

[krallin]

Hi there,

This syntax won't work on Python 2.6, mind using {0} instead of {}?

Just logging the exception might not be extremely useful though. Can you share the type of output this produces for you? It might be better to just include the stacktrace in the error line with exc_info=True.

Cheers,

[jammon]

Oh sure, {0} is better.

I had errors in my json-file (no outer braces, spurious comma, things like that) and got just a Unable to load configuration at config.json. At first I suspected some problem with file access, wrong working directory or similar problems. When I changed the code like this, I got Extra data: line 1 column 14 - line 9 column 1 (char 13 - 77) or Expecting property name: line 9 column 1 (char 78). Then I could fix my config.json.

Maybe for others the stacktrace might be useful, but for me, I was happy with just the exception.

[krallin]

@jammon

Can you try with exc_info = True in the earlier logging call and check whether the output is as useful?

Alternatively, we might want to special case ValueError (and use your error message), and fall back to a stacktrace otherwise.

Thoughts?

[krallin]

Also, in practice we should probably update both these calls to not use string formatting as well and use:

logger.error("foo %s", bar)

[jammon]

Please change the code to your liking, I'm sure you know more of it than I do. I just wanted to fix a problem that bit me.

Where do I have to put the exc_info = True? (I will come back tomorrow, private duties.)

[jammon]

Obviously you are right about the ValueError. Are there any exceptions left that could possibly be raised? I don't think so. When talking about the code, I read it more closely. Do you mind renaming ConfigFile._dirty to ConfigFile._unsaved? I think this conveys better the meaning of this property.

[krallin]

A few things:

• I'd rather we don't rename _dirty to _unsaved. "Dirty" is very standard for something that was changed and not saved yet, but more importantly, we should avoid making changes that are purely cosmetic (someone might depend on that variable — obviously they shouldn't —, but let's not change it just for the sake of changing it).
• Can you use ValueError as e instead of ValueError, e? ValueError, e won't work on Python 3, which we're working on supporting (https://github.com/koenbok/Cactus/pull/115)
• Can you try and add tests for this?

[jammon]

I understand your point about _dirty, although I don't consider it a purely cosmetic change. Clarity of names is a plus for somebody who is new to the project. But I don't want to be pedantic about that, so I changed it back.

Thanks for the hint on ValueError as e. I wrote tests, but I had to introduce a new dependency for the tests (testfixtures).

[krallin]

Hey there,

I pulled this in with some changes (d5e3877ccbf9ba5aab072a63d24a51d0bd5a50ed). Thanks for the work here!

Cheers,