search

duct-framework / logger.timbre

Integrant methods for the Timbre logging library
3 stars 2 forks source link

Set global logging config on init-key #2

Closed RickMoynihan closed 7 years ago

RickMoynihan commented 7 years ago

Fixes issue #1.

weavejester commented 7 years ago

Can you remove b534f81? Changing version numbers shouldn't be part of this PR.

RickMoynihan commented 7 years ago

Yes, happy to do that.

I think I've figured out the problem with the tests, my editors set to strip whitespace from end of lines, and I think it's stripped "\ " from the regexes.

weavejester commented 7 years ago

The :duct.logger.timbre/set-config! key should be something more like: :set-root-binding?.

RickMoynihan commented 7 years ago

This should now have all the initially requested changes.

Not sure what you want to do with it though given the discussions on #1 and #3.

Also is it worth making the regexes more robust with something like /d/d for the line numbers? It's pretty confusing to break all the tests by just adding a new require.

weavejester commented 7 years ago

It might be worth doing that for the regular expressions. Initially I wasn't expecting this library to change much, so while the tests are brittle, they do ensure that the line numbers are logged correctly. However, if we're going to be doing a bit of work, perhaps it would be better to use more general regexs so it's not a pain to change each time. Maybe we could also move around the "\ " characters so they don't fall on a line ending. Perhaps both of those changes fall under the same commit.

With regard to the ::set-root-binding? key, I meant you should remove the namespace to make it consistent with the other options.

Let's keep this PR how it is and add the legacy appender middleware as a separate PR, so we don't have too much in one PR. However, let's remove the changes to the README, since that information will have to change when we put the legacy appender in anyway.

Also could you remove lines 88, 94, 97 and 99 in the test file? I don't think the extra comments/docstrings are necessary, as to me the test is obvious enough without them.

RickMoynihan commented 7 years ago

Ok extra changes pushed for review. Will make README changes after lunch.

RickMoynihan commented 7 years ago

README changes removed.

weavejester commented 7 years ago

This looks ready for merging. Can you rebase and squash your commits into two: one for the changes to the regular expressions, and one for the changes to the root logger.

I use the seven rules as a guideline for commit messages. So perhaps something like:

Make test regexes more robust

And:

Set global logging config on init-key
RickMoynihan commented 7 years ago

Squashed.

weavejester commented 7 years ago

Perfect, thanks!