Skip to content

remove color from log files - #99

Open
mainrs wants to merge 3 commits into
Qix-:masterfrom
mainrs:remove_color_from_logfiles
Open

remove color from log files#99
mainrs wants to merge 3 commits into
Qix-:masterfrom
mainrs:remove_color_from_logfiles

Conversation

@mainrs

@mainrs mainrs commented Sep 4, 2019

Copy link
Copy Markdown

This PR removes the color from logfiles while keeping them if they are outputted to the console. This is done by additionally filtering for FileHandlers during the hook call.

I also added a new test that explicitly creates a log file. The file doesn't contain any ANSI codes after the test has run.

I am not sure if the output has to be re-generated. Best would be if you could take a look before merging it. If they do need ot re-generate, let me know. I'll add them in a new commit.

Closes #87.


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

Sven Lechner added 3 commits September 4, 2019 19:37
Before this commit, the test only logged to a file. It now logs to stdout too to allow tests to detect if it still uses ANSI on stdout
This has to be done as the new tests appends some output to each file
@mainrs

mainrs commented Sep 12, 2019

Copy link
Copy Markdown
Author

@Qix- I re-generated the tests after taking a closer look at them. I have no idea why 3 still fail. I am pretty sure I did everything correctly when generating them. Mind taking a closer look?

@mainrs

mainrs commented Jul 21, 2020

Copy link
Copy Markdown
Author

I'll close the issue as there is apparently no interest in merging the changes right now.

@mainrs mainrs closed this Jul 21, 2020
@Qix-

This comment was marked as outdated.

@mainrs

mainrs commented Jul 21, 2020

Copy link
Copy Markdown
Author

I was cleaning up my PR section on GitHub as I use it to keep an overview on stuff and it clutters a lot over time. No problem, I can re-open it.

@mainrs mainrs reopened this Jul 21, 2020
@Qix-

Qix- commented Jul 21, 2020

Copy link
Copy Markdown
Owner

Out of experience of almost (over? dunno) 10 years on the platform - that's a lost cause, and you're only going to annoy other maintainers as well. The issues/pr tabs aren't a good tool for that - I'm surprised Github has kept them around all these years.

OSS evolves both rapidly and very slowly, depending on the project. Prepare for some of your PRs to go unnoticed for years and then randomly merged after a while.

@mainrs

mainrs commented Jul 22, 2020

Copy link
Copy Markdown
Author

Sorry, I didn't meant to annoy or upset you in any way. The tabs worked (at least for me) great now. I'd take recommendations for other tools/interfaces to manage issues and PRs though :) They bulk up a lot over time and it's hard to keep track of them when not closing some of them after a long period of time.

@Qix- Qix- left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small formatting nit, let's see if it still works. If you fix it and then rebase off master, I'll merge right away and award you the bounty. I can help you do this if need be :)

I'm really sorry about it taking so long - everything happening this year has been a huge distraction.

Comment thread better_exceptions/log.py
Comment on lines +17 to +18
patchables = [handler() for handler in logging._handlerList if isinstance(handler(), StreamHandler)
if not isinstance(handler(), FileHandler)]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
patchables = [handler() for handler in logging._handlerList if isinstance(handler(), StreamHandler)
if not isinstance(handler(), FileHandler)]
patchables = [
handler() for handler in logging._handlerList
if (
isinstance(handler(), StreamHandler) and
not isinstance(handler(), FileHandler)
)
]

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if this works still, as it's a bit clearer to read.

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.

Logged exceptions shouldn't contain colour

2 participants