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

Fix #3297 where caplog.clear() did not clear text, just records #3301

Merged
merged 3 commits into from
Mar 13, 2018

Conversation

ankostis
Copy link
Contributor

@ankostis ankostis commented Mar 12, 2018

Fix #3297 where caplog.clear() did not clear text, just records.
Fix by @Thisch that re-installs a new StringIO on the handler as suggested in https://stackoverflow.com/questions/4330812/how-do-i-clear-a-stringio-object#4330829

  • Actually work delegated to a new reset() method on the handler, used also on caplog construction .
  • Added TC demonstrating failure.

@coveralls
Copy link

coveralls commented Mar 12, 2018

Coverage Status

Coverage increased (+0.06%) to 92.6% when pulling 02bec7a on ankostis:caplog_clear_text into 3909225 on pytest-dev:master.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ankostis!

The only thing missing is a changelog entry, please create a file named changelog/3297.bugfix.rst describing this change, I suggest:

``caplog.clear`` now also properly clears the ``text`` property.

@@ -176,6 +176,10 @@ def emit(self, record):
self.records.append(record)
logging.StreamHandler.emit(self, record)

def reset(self):
Copy link
Member

Choose a reason for hiding this comment

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

For consistency perhaps we should call this clear too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But i see that there pre-existed also _LiveLoggingStreamHandler.reset() method,
so maybe @Thisch had a reason to use this name.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I didn't notice that, never mind then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the docstring of LogCaptureFixure.clear starts with "reset" ;) There is no specific reson why I chose "reset" in my initial commit, though.

@ankostis
Copy link
Contributor Author

I didn't see any other files in changelog/ so i hesitated to add this.
I've re-written last commit, and hope this is what you asked.

@nicoddemus
Copy link
Member

I didn't see any other files in changelog/ so i hesitated to add this.
I've re-written last commit, and hope this is what you asked.

Yep is just that we just had a release last week and your PR is the first one since then to land on master. 👍

Thanks, LGTM! 👍

@RonnyPfannschmidt RonnyPfannschmidt merged commit 2612d96 into pytest-dev:master Mar 13, 2018
@RonnyPfannschmidt
Copy link
Member

👍 thanks

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.

5 participants