Disable qtlog #58
Merged
Disable qtlog #58
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -43,6 +43,8 @@ For example: | |||
| 1 failed in 0.01 seconds | |||
|
|
|||
|
|
|||
| **Disabling Loggin Capture** | |||
|
The diff looks good apart from the few inline comments. I'll test this with qutebrowser later. |
| qInstallMsgHandler(self._previous_handler) | ||
|
|
||
| @contextmanager | ||
| def disabled(self): |
There was a problem hiding this comment.
Perhaps call this disable instead of disabled? Doing qtlog.disable() seems more natural, disabled sounds more like an attribute to check whether something is currently disabled.
There was a problem hiding this comment.
Hmm perhaps... I named it from "disabled within the context".
Either way, I think it is too late to rename it now without breaking semantic versioning... 😶
|
I now tried it and got this: full output with some |
|
Argh, I think I know what's going on... I had to create a thin wrapper so I will work on and the other points of your review tomorrow. Thanks! 😄 |
nicoddemus
force-pushed
the
disable-qtlog
branch
from
eadb2de
to
59f9396
Compare
|
Unfortunately I couldn't reproduce the error you had in Is this failing in a different branch perhaps? Either way, I've removed the thin wrapper over |
|
Hey @The-Compiler, did you had a chance of checking if this last changes fixes the problem you were having? Or if you point me to a branch in |
|
I now tried again (in the logtest branch) and couldn't reproduce it either. However one test still fails (the one you showed above) which passed with pytest-qt 1.3.0. I don't really see why, but I haven't debugged it so far. |
|
Oh ok, I will take a look and see if it's a problem with the test or with |
|
Hey @The-Compiler, finally could tackle this, sorry for taking this long. I have figured out why
I hope all my explanation makes sense. 😄 In summary, the problem is that there's a conflict between I see a couple of options:
Let me know what you think, I would be glad to help with a PR or otherwise. 😄 Cheers, |
|
Thanks for the great explanation, it makes total sense! 😉 I already fixed the tests a while ago by using the loggers directly instead of qWarning() since what I actually want to test is the log filtering anyways. So I think this can be merged now (at least from my POV), in the hope someone other than me will find it useful 😄 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@The-Compiler, can you do a quick review? Perhaps even test it with your code base to ensure everything is like you wanted. 😄