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

Add support for per-loglevel formatting #32

Merged
merged 8 commits into from May 24, 2016
Merged

Conversation

mdraw
Copy link
Contributor

@mdraw mdraw commented May 21, 2016

This makes it possible to specify a formatter dictionary that enables colorlog to format messages differently based on their log level.

Use case:

  • If you want to log informational output using logger.info(), you are rarely interested in the module and line number where the message comes from.
  • On the other hand, it can be useful to prefix warnings, errors and criticals with 'WARNING: ' etc. and add line number and module at the end, for example.

This is why I added the level_fmts argument to ColoredFormatter.

I tested the new code with Python 2.7 and Python 3.5.

Possible issues:

  • default_level_formats has nothing to do with colorlog's normal default_formats. Could this be confusing?
  • We could reuse the fmt argument instead of introducing the new level_fmts. We would need to check if fmt is a dict or a string in ColoredFormatter.__init__() and could use it differently depending on the type. I don't know if that would be better. What is your opinion?

@borntyping
Copy link
Owner

This is pretty cool, though is quite far from the modules original scope. I think this behaviour might be better placed in a subclass of the ColoredFormatter class, which is already quite complex.

default_level_formats has nothing to do with colorlog's normal default_formats. Could this be confusing?

Not worried about the naming of default_level_formats - it's an internal constant that users shouldn't see anyway. It would be good to have the formats defined it resemble those in default_formats though, as they currently have a very different layout of the log messages. It might be easier to go a step further and not include a set of defaults, since it's a very user-specific function.

We could reuse the fmt argument instead of introducing the new level_fmts. We would need to check if fmt is a dict or a string in ColoredFormatter.init() and could use it differently depending on the type. I don't know if that would be better. What is your opinion?

I like this idea of reusing fmt, that would keep the API much simpler.

@mdraw
Copy link
Contributor Author

mdraw commented May 23, 2016

Thanks for the feedback! I hope you don't mean it is too far from your original idea of colorlog to get merged.

I think this behaviour might be better placed in a subclass of the ColoredFormatter class, which is already quite complex.

Actually, this is how I had originally implemented it, but I decided to merge it into ColoredFormatter itself to avoid duplicating code from the base class (I would have to copy-paste most of the ColoredFormatter.format() and ColoredFormatter.__init__() functions to my new class, because my new code has to be in the middle of the format() and __init__() functions).

It might be easier to go a step further and not include a set of defaults, since it's a very user-specific function.

So should I just remove the whole default_level_formats dict from the code? Should we re-add it as an example in the documentation then?

I like this idea of reusing fmt, that would keep the API much simpler.

OK, I'll rewrite it to reuse fmt and remove the new separate level_fmts argument.

If fmt is a dict, it is now used as a mapping of
log levels to different formatters.

default_level_formats is no longer used internally.
@borntyping
Copy link
Owner

Yep - happy to use this in colorlog, I'd just greatly prefer to put it in a subclass.

I would have to copy-paste most of the ColoredFormatter.format() and ColoredFormatter.__init__() functions to my new class, because my new code has to be in the middle of the format() and __init__() functions

I think __init__ doesn't need so many changes if we drop default_level_formats and use a single fmt argument. An approach to format() might be to add some optional arguments that override the format/style (so that the subclass can call it with a specific format).

@mdraw
Copy link
Contributor Author

mdraw commented May 23, 2016

I was a bit confused about when I have to call the superclass functions at first, but I've just managed to write my new code exclusively into a subclass without duplicating any code from ColoredFormatter.

Do you have a suggestion how to name this new class? I chose "ColoredLevelFormatter" for now, but that might sound a bit awkward.

@borntyping
Copy link
Owner

Maybe just colorlog.LevelFormatter? Everything else looks great.

@mdraw
Copy link
Contributor Author

mdraw commented May 24, 2016

colorlog.LevelFormatter sounds good, I just renamed it. Would you like to merge the PR in this state?

@borntyping borntyping merged commit 4e0bc7d into borntyping:master May 24, 2016
@mdraw
Copy link
Contributor Author

mdraw commented May 24, 2016

Thanks a lot for merging this!
I'd like to remind you to update https://github.com/borntyping/python-colorlog/blob/master/colorlog/colorlog.py#L158 before tagging a new release.

@borntyping
Copy link
Owner

Thanks for all the work you've put in :)

borntyping added a commit that referenced this pull request Feb 6, 2017
Add support for per-loglevel formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants