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

Add str() support to LineMatcher #8050

Merged
merged 8 commits into from Nov 21, 2020
Merged

Add str() support to LineMatcher #8050

merged 8 commits into from Nov 21, 2020

Conversation

mcsitter
Copy link
Contributor

Added __str__ method to LineMatcher class.

The LineMatcher class is used in the RunResult class, which has the stdout attribute, which is an instance of LineMatcher. result.stdout.str() is used 66 times in the codebase. The use of the str() function is mentioned here as well. If I should make any other changes. Since .str() is used throughout the codebase, changing everything to str(matcher) seems pointless.

If you think a test should be added, let me now, however it is implicitly tested in the Pytester tests.

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst.

Closes #1265

Copy link
Member

@bluetech bluetech left a 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

changelog/1265.improvement.rst Outdated Show resolved Hide resolved
src/_pytest/pytester.py Outdated Show resolved Hide resolved
src/_pytest/pytester.py Outdated Show resolved Hide resolved
mcsitter and others added 3 commits November 19, 2020 11:05
Co-authored-by: Ran Benita <ran@unusedvar.com>
Co-authored-by: Ran Benita <ran@unusedvar.com>
Co-authored-by: Ran Benita <ran@unusedvar.com>
@mcsitter
Copy link
Contributor Author

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 :func:`stdout.str() <LineMatcher.str()>` ? I figure that is the part that needs to change.
:func:`str(stdout) <LineMatcher.__str__()>` ? Would that reference work?

src/_pytest/pytester.py Outdated Show resolved Hide resolved
@bluetech
Copy link
Member

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.

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 :)

What would you put instead of :func:stdout.str() <LineMatcher.str()> ? I figure that is the part that needs to change.
:func:str(stdout) <LineMatcher.__str__()> ? Would that reference work?

Right, I think that's the correct incantation, but only sphinx can tell :)

BTW in the Checks section of every PR there is a docs/readthedocs.org:pytest link to the rendered docs, so you can check that the reference works even without testing locally if you prefer.

@nicoddemus
Copy link
Member

If you think a test should be added, let me now, however it is implicitly tested in the Pytester tests.

I really think a test is needed, mostly to ensure we don't regress this in the future by accident.

@mcsitter
Copy link
Contributor Author

  1. I updated the reference. Have a look at it here.

  2. I added a test.

@nicoddemus nicoddemus changed the title Fix LineMatcher API Add str() support to LineMatcher Nov 21, 2020
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.

LGTM!

@bluetech bluetech merged commit 0cef530 into pytest-dev:master Nov 21, 2020
@mcsitter mcsitter deleted the fix-1265-linematcher-str branch November 22, 2020 13:37
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.

pytester linematcher - less error prone api
3 participants