-
-
Notifications
You must be signed in to change notification settings - Fork 204
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
base: master
Are you sure you want to change the base?
Conversation
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
@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? |
I'll close the issue as there is apparently no interest in merging the changes right now. |
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. |
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. |
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. |
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. |
There was a problem hiding this 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.
patchables = [handler() for handler in logging._handlerList if isinstance(handler(), StreamHandler) | ||
if not isinstance(handler(), FileHandler)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) | |
) | |
] |
There was a problem hiding this comment.
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.
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