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

Remove ColoredRecord class #52

Closed
wants to merge 1 commit into from
Closed

Conversation

noamkush
Copy link

@noamkush noamkush commented Nov 4, 2017

This intends to solve a rare problem I had encountered.
I use both colorlog and multiprocessing-logging. I noticed that in the subprocesses, if I call logger.exception, I do get the message but not the traceback. I tracked down the issue to this line, where multiprocessing-logging calls format(record), expecting the formatter to set record.exc_text. Since the ColoredFormatter wraps the record before calling the base class, the original record doesn't get it's exc_text set, and I lose my traceback.
I think that this issue is much easier to fix in colorlog. I removed the ColoredRecord and instead just set the escape code properties to the original record.

@borntyping
Copy link
Owner

In theory, accessing record.exc_text should to access the same attribute on the original record via __getattr__ on the ColouredRecord class (https://github.com/noamkush/python-colorlog/blob/eb6e2dca59c397586f5d5c3f48923da3d70a6127/colorlog/colorlog.py#L47-L48). Any idea why that doesn't work?

@noamkush
Copy link
Author

noamkush commented Nov 4, 2017

Acessing record.exc_text would use __getattr__ but assigning a new value would use __setattr__.

@borntyping
Copy link
Owner

Ah, I misread the code you linked. I'd prefer to add __setattr__ if possible - my original intent with ColoredRecord is so that records don't get modified by colorlog (especially in setups with multiple handlers).

@noamkush
Copy link
Author

noamkush commented Nov 4, 2017

Well, if you're also implementing __setattr__, you're modifying the record anyway.
I think the property names won't be used by other handlers.

@borntyping
Copy link
Owner

__setattr__ would only be used when another library modifies the record (i.e. it shouldn't pass through the __record attribute). I know the property names are very unlikely to be used elsewhere, but it'll be horrible to debug they do, such as a custom record class with a .reset() method.

@noamkush
Copy link
Author

noamkush commented Nov 5, 2017

I tried to implement it with __setattr__ but it becomes an unworkable mess of __dict__ uses and infinite recursion. I think I'll just override the format method in my code.

@noamkush noamkush closed this Nov 5, 2017
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