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

Fixed #27392 -- Removed "Tests that", "Ensures that", etc. from test docstrings. #7439

Merged
merged 1 commit into from Nov 11, 2016

Conversation

Projects
None yet
5 participants
@za
Copy link
Contributor

commented Oct 27, 2016

Django ticket: #27392

@@ -53,6 +53,9 @@ Python style
(``six.assertRaisesRegex()`` as long as we support Python 2) only if you need
to use regular expression matching.

* In test docstrings, state the expected behaviour that each test demonstrates.

This comment has been minimized.

Copy link
@felixxm

felixxm Oct 27, 2016

Member

behaviour -> behavior

@timgraham

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

Maybe it wasn't clear from the ticket, but I'd like to fix existing docstrings so that contributors don't copy the pattern.

@timgraham timgraham changed the title #27392 Remove "Tests that", "Ensures that", etc. from test docstings Fixed #27392 -- Removed "Tests that", "Ensures that", etc. from test docstings. Oct 27, 2016

@za

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

Yes, I'll fix the existing docstrings as well.

@@ -3300,7 +3300,7 @@ def test_custom_function_action_with_redirect(self):

def test_default_redirect(self):
"""
Test that actions which don't return an HttpResponse are redirected to
Actions which don't return an HttpResponse are redirected to

This comment has been minimized.

Copy link
@timgraham

timgraham Oct 28, 2016

Member

It would be nice to reflow the docstrings in cases like this (wrapping at 79 characters) so that the lines aren't unbalanced.

This comment has been minimized.

Copy link
@za

za Oct 30, 2016

Author Contributor

@timgraham OK, I'll fix it.

@@ -174,7 +174,7 @@ def test_check_migrations(self):

class CommandRunTests(AdminScriptTestCase):
"""
Tests that need to run by simulating the command line, not by call_command.
Need to run by simulating the command line, not by call_command.

This comment has been minimized.

Copy link
@timgraham

timgraham Oct 28, 2016

Member

This is using "Tests" as a noun, not as a verb, so it's okay as is.

@za za changed the title Fixed #27392 -- Removed "Tests that", "Ensures that", etc. from test docstings. Fixed #27392 -- Removed "Tests that", "Ensures that", etc. from test docstrings. Oct 30, 2016

@za

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2016

For a recap:

$ ack-grep "Tests that" tests/
tests/user_commands/tests.py
177:    Tests that need to run by simulating the command line, not by call_command.

This Tests that as a noun.

$ ack-grep "Test that" tests/
$ ack-grep "Ensures that" tests/
$ ack-grep "Ensure that" tests/
$ ack-grep "Checks that" tests/
$ ack-grep "Check that" tests/
tests/apps/tests.py
81:        msg = "Cannot import 'there is no such app'. Check that 'apps.apps.NoSuchApp.name' is correct."

tests/auth_tests/test_views.py
800:            "Redirection loop for authenticated user detected. Check that "

These Check that are not part of docstrings.

@timgraham I'm done and waiting for review.

@za

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

I just read working-with-git doc. I can make a better PR, if only I read that doc first.

@thatdocslady

This comment has been minimized.

Copy link

commented Nov 5, 2016

@za I ran a more generic search on the /test/ directory in master (used " that " with spaces instead of the more specific "Tests that", etc...) and found other occurrences that you might have missed (i.e. "Verifies that"... etc).

If you don't mind running the more generic search and fixing the remaining strings, and also squashing your commits and updating the PR, you can make this PR more checkin-ready.

@za

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2016

Hi @thatdocslady ! Yes, sure I'll be happy to make a PR which is more checkin-ready. I have a question. When there was an update on master, instead of:

(ticket_27392) $ git fetch upstream
(ticket_27392) $ git rebase

I was doing:

(master) $ git pull upstream master
(master) $ git checkout ticket_27392
(ticket_27392) $ git merge master

Should I re-work my PR (make another new branch and make another changes) because of this? cc: @timgraham

@timgraham

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

Now that GitHub has a squash and merge button, squashing is less critical so feel free to add more commits as needed. Next time, try using rebase rather than merge though.

@za

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2016

@timgraham ack

@za

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2016

@thatdocslady I've removed "Verifies that" in tests folder and also add "Verifies that" in coding-style.txt. I think I screwed up the commits, hopefully GitHub squash commits will solve this. Just let me know if there's something I miss.

CC: @timgraham

@timgraham timgraham force-pushed the za:ticket_27392 branch 2 times, most recently from e81d0f8 to fd8e06d Nov 10, 2016

@timgraham timgraham force-pushed the za:ticket_27392 branch from fd8e06d to 321e94f Nov 11, 2016

@timgraham timgraham merged commit 321e94f into django:master Nov 11, 2016

6 checks passed

default Build finished.
Details
docs Build finished.
Details
flake8 Build finished.
Details
isort Build finished.
Details
javascript Build finished.
Details
windows Build finished.
Details
@timgraham

This comment has been minimized.

Copy link
Member

commented Nov 11, 2016

Thanks for your work. I had to proofread the patch fairly carefully as there were some sentences that weren't reworded correctly. I left a comment on the ticket about some additional phrases that could be fixed up. If you work on it, please take a look at the committed patch to get a sense of my edits so I don't have to do similar cleanups again.

@za

This comment has been minimized.

Copy link
Contributor Author

commented Nov 11, 2016

@timgraham thanks for your proofread as well! OK, I'll have a look the ticket.

@mazulo

This comment has been minimized.

Copy link

commented Feb 17, 2017

Hey @timgraham AFAIK the work of the related ticket isn't finished (the ticket is open yet) but this PR is merged. Is there another PR with this work finished or is this in "standby" until the new Django version is released? By the way I'd be happy if I can help on this :)

@timgraham

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

#7777 has done a little, it would be fine to continue this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.