-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add str() support to LineMatcher #8050
Add str() support to LineMatcher #8050
Conversation
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.
Thanks @mcsitter, I left some comments.
We should also update this sentence to refer to str(...)
instead of .str()
IMO: https://docs.pytest.org/en/latest/reference.html#pytest.pytester.RunResult.stdout
Co-authored-by: Ran Benita <[email protected]>
Co-authored-by: Ran Benita <[email protected]>
Co-authored-by: Ran Benita <[email protected]>
Thanks for the feedback! Not much to discuss about the requested changes. I will try to formulate better changelog entries in the future and make myself more familiar with sphinx directives. Is there a sensible guideline for when to use them? There seem to be so many, that it could clutter up things quite a bit. What would you put instead of |
I like hyperlinks so I tend to add them when someone might be inclined to follow them. But there are not strict guidelines, just depends on personal preference and laziness :)
Right, I think that's the correct incantation, but only sphinx can tell :) BTW in the Checks section of every PR there is a |
I really think a test is needed, mostly to ensure we don't regress this in the future by accident. |
|
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.
LGTM!
Added
__str__
method to LineMatcher class.The
LineMatcher
class is used in theRunResult
class, which has thestdout
attribute, which is an instance ofLineMatcher
.result.stdout.str()
is used 66 times in the codebase. The use of thestr()
function is mentioned here as well. If I should make any other changes. Since.str()
is used throughout the codebase, changing everything tostr(matcher)
seems pointless.If you think a test should be added, let me now, however it is implicitly tested in the
Pytester
tests.changelog
folder, with a name like<ISSUE NUMBER>.<TYPE>.rst
.Closes #1265