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 color from log files #99

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mainrs
Copy link

@mainrs mainrs commented Sep 4, 2019

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
Copy link
Author

mainrs commented Sep 12, 2019

@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
Copy link
Author

mainrs commented Jul 21, 2020

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-
Copy link
Owner

Qix- commented Jul 21, 2020

I don't understand why people close PRs because maintainers are busy. I'm starting a company and clearly the other maintainer is also busy. There is of course interest, but by closing this PR it leaves me no choice but to disregard your changes. You've made that decision for me.

Feel free to re-open the PR. You're not losing anything by doing so. I will get to this when I have more time.

@mainrs
Copy link
Author

mainrs commented Jul 21, 2020

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-
Copy link
Owner

Qix- commented Jul 21, 2020

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
Copy link
Author

mainrs commented Jul 22, 2020

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.

Copy link
Owner

@Qix- Qix- left a comment

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 on lines +17 to +18
patchables = [handler() for handler in logging._handlerList if isinstance(handler(), StreamHandler)
if not isinstance(handler(), FileHandler)]
Copy link
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
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