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

Move docstring to appropriately placed comment #6933

Merged
merged 1 commit into from
Sep 1, 2019

Conversation

pradyunsg
Copy link
Member

No description provided.

@pradyunsg pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Aug 27, 2019
@@ -2,7 +2,6 @@


def test_environ(script, tmpdir):
"""$PYTHONWARNINGS was added in python2.7"""
Copy link
Member

Choose a reason for hiding this comment

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

I think this was meant to describe what the test is doing. Moving this below makes it seem like the test is meaningful on its own and PYTHONWARNINGS was added later. I would update this docstring to be e.g. "Test PYTHONWARNINGS, as added in python2.7" or remove the docstring and update the test name to test_PYTHONWARNINGS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea... However, given that 2.7 is the lowest version we have, it doesn't describe much. Anyway, IMO this is one of the doc-strings that don't add value by being there (it doesn't describe the test).

We should do follow up PR, adding a more useful docstring -- I'll merge, since test_warnings.py::test_environ probably explains enough about this test, for now?

@pradyunsg pradyunsg merged commit d764181 into pypa:master Sep 1, 2019
@pradyunsg pradyunsg deleted the misc/warning-cleanup branch September 1, 2019 16:25
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Oct 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants