Fixed #27656 -- Updated docstring verbs according to PEP 257. #7763

desecho · 2016-12-29T16:16:33Z

https://code.djangoproject.com/ticket/27656

timgraham · 2016-12-29T16:21:54Z

django/test/client.py

-        and passes to the handler, returning the result of the handler.
-        Assumes defaults for the query environment, which can be overridden
+        The master request method. Compose the environment dictionary
+        and pass to the handler, return the result of the handler.


timgraham · 2016-12-29T16:23:28Z

django/test/client.py

@@ -674,9 +674,9 @@ def _login(self, user, backend=None):

    def logout(self):
        """
-        Removes the authenticated user's cookies and session object.
+        Remove the authenticated user's cookies and session object.


"Log out the user by removing the cookies and session object."
(remove second sentence)

timgraham · 2016-12-29T16:24:21Z

django/test/client.py

@@ -701,7 +701,7 @@ def _parse_json(self, response, **extra):
        return response._json

    def _handle_redirects(self, response, **extra):
-        "Follows any redirects by requesting responses from the server using GET."
+        "Follow any redirects by requesting responses from the server using GET."


Pep257 says, "Triple quotes are used even though the string fits on one line. This makes it easy to later expand it."

timgraham · 2016-12-29T16:26:05Z

django/test/client.py

@@ -441,14 +441,14 @@ def __init__(self, enforce_csrf_checks=False, **defaults):

    def store_exc_info(self, **kwargs):
        """
-        Stores exceptions when they are generated by a view.
+        Store exceptions when they are generated by a view.
        """
        self.exc_info = sys.exc_info()

    @property
    def session(self):
        """


Should these by one liners ? e.g. """Obtain the current session variables.""" -- we currently don't have consistency about that.

desecho · 2016-12-29T17:18:45Z

@timgraham I'll be making the changes you suggested as well as fixes for the verb style. Also, I noticed that this coding style is not being followed as well. I should probably be fixing that as well - I go through the all of the comments anyway.

In test docstrings, state the expected behavior that each test demonstrates. Don’t include preambles such as “Tests that” or “Ensures that”.

And I assume it is a good idea to also make sure these PEP rules are being followed as well:

Triple quotes are used even though the string fits on one line. This makes it easy to later expand it.
The closing quotes are on the same line as the opening quotes. This looks better for one-liners.
There's no blank line either before or after the docstring. (except for classes)
The docstring is a phrase ending in a period.

timgraham · 2016-12-29T17:23:02Z

Keep this patch focused on django/ only -- don't worry about test docstrings, there's a separate ticket for that.
The PEP rules you mentioned are fine.

desecho · 2016-12-29T17:27:23Z

Ok. Also I noticed that the wrapping is sometimes different. It can be 72 or less than 79. Since you mentioned it in the commit we should probably make it consistent to 79.

Documentation, comments, and docstrings should be wrapped at 79 characters, even though PEP 8 suggests 72.

timgraham · 2016-12-29T17:29:11Z

If you're making significant changes to the docstring, feel free to adjust the length to 79, otherwise don't make needless changes.

desecho · 2016-12-29T17:46:15Z

I made some changes. Please see if I am on the right track.

timgraham · 2016-12-29T18:43:34Z

tests/admin_checks/tests.py

@@ -158,7 +158,7 @@ class SongAdmin(admin.ModelAdmin):

    def test_custom_modelforms_with_fields_fieldsets(self):
        """
-        # Regression test for #8027: custom ModelForms with fields/fieldsets


As I mentioned before, please don't modify any test docstrings for this PR.

timgraham · 2016-12-29T18:46:39Z

django/test/client.py

-    The test client has been asked to follow a redirect loop.
-    """
+    """The test client has been asked to follow a redirect loop."""
+


I'd skip the blank line after the class docstring changes. Focus on changing the verbs only.

timgraham · 2016-12-29T18:47:19Z

django/contrib/admin/templatetags/admin_list.py

-    # reporting purposes. Needed to maintain backwards
-    # compatibility with existing admin templates.
+    """
+    Wrapper class used to return items in a list_editable


Might as well reflow the line length in a case like this.

timgraham · 2016-12-29T18:48:47Z

django/contrib/admin/checks.py

@@ -420,8 +419,8 @@ def _check_prepopulated_fields_value(self, obj, model, val, label):
            ]))

    def _check_prepopulated_fields_value_item(self, obj, model, field_name, label):
-        """ For `prepopulated_fields` equal to {"slug": ("title",)},
-        `field_name` is "title". """
+        """For `prepopulated_fields` equal to {"slug": ("title",)},


Be consistent about the multiline style -- use

""" .... """

timgraham · 2016-12-29T18:49:27Z

django/apps/registry.py

-        # perform imports because of the risk of import loops. It mustn't
-        # call get_app_config().
+        """
+        Since this method is called when models are imported, it cannot


This content isn't a docstring, so inline comment is more appropriate.

timgraham · 2016-12-29T18:50:22Z

django/apps/config.py

-    """
-    Class representing a Django application and its configuration.
-    """
+    """Class representing a Django application and its configuration."""


If there aren't any content changes required, don't bother changing a docstring like this.

timgraham · 2016-12-30T21:10:02Z

Please do only verb changes in this PR. I think all other changes could be refactored automatically with some tool if we decide to do that. Otherwise, this is just creating a huge unreviewable patch.

desecho · 2016-12-30T21:21:56Z

So I assume this means don't follow your previous suggestions and do only verb changes. I caught few typos - I will include them as well.

timgraham · 2016-12-30T21:25:26Z

Yes, sorry I had second thoughts about this when I started seeing how many more changes would be required as well as the thought that other changes could be automated.

timgraham · 2016-12-31T01:47:11Z

On further thought, I'd hold off on doing further changes for a couple weeks so that you don't create merge conflicts with all that code that will be removed in Django 2.0 as part of the ending deprecations (#7773). I'll leave a note when it's a good time to continue. Thanks!

desecho · 2017-01-21T22:17:46Z

I guess we can continue with this ticket since #7773 got merged.

timgraham · 2017-01-24T00:55:34Z

Sure, feel free to rebase. Maybe you can submit some smaller commits for easier review, such as only doing "django.contrib" so that the patch doesn't get so long that GitHub starts displaying "Load diff" buttons. It gets tedious to click those to review things.

desecho · 2017-01-25T19:18:00Z

I completed all of the changes.

timgraham · 2017-02-02T15:44:41Z

What was your methodology for identifying places that need to be fixed? Working my way through contrib.admin (so don't bother reviewing that one) I see a lot of places where, for example, the first verb in the sentence was fixed but not the second (e.g. "Remove the authenticated user's ID from the request and flushes their". Even grepping for "Returns" shows places that weren't updated. Another phrasing to check for is something like "If no user is retrieved an instance of AnonymousUser is returned." which should be "If no user is retrieved, return an instance of AnonymousUser." I know this is really tedious, but reviewing a partially completed attempt at this isn't fun either.

timgraham · 2017-02-04T21:43:26Z

django.contrib commit merged in 5411821 (with many edits).

timgraham reviewed Dec 29, 2016

View reviewed changes

desecho force-pushed the ticket_27656 branch from 4881036 to 3217f1b Compare December 29, 2016 16:32

desecho force-pushed the ticket_27656 branch 2 times, most recently from 247e63a to 35785e9 Compare December 29, 2016 17:45

desecho force-pushed the ticket_27656 branch 4 times, most recently from 0be176f to c6dbc53 Compare December 29, 2016 18:21

timgraham reviewed Dec 29, 2016

View reviewed changes

desecho force-pushed the ticket_27656 branch from c6dbc53 to 8efc72b Compare December 29, 2016 19:20

desecho force-pushed the ticket_27656 branch 2 times, most recently from 197a583 to e68078c Compare January 5, 2017 19:49

desecho changed the title ~~Fixed #27656 -- Fix comment verb style according to PEP 257.~~ [WIP] Fixed #27656 -- Fix comment verb style according to PEP 257. Jan 6, 2017

desecho force-pushed the ticket_27656 branch from e68078c to cd80b86 Compare January 24, 2017 20:32

desecho changed the title ~~[WIP] Fixed #27656 -- Fix comment verb style according to PEP 257.~~ Fixed #27656 -- Fix comment verb style according to PEP 257. Jan 25, 2017

desecho force-pushed the ticket_27656 branch from 6174f17 to e31d965 Compare January 25, 2017 19:17

desecho force-pushed the ticket_27656 branch from e31d965 to 64ca24a Compare January 30, 2017 18:07

timgraham mentioned this pull request Feb 2, 2017

Fixed inconsistent docstring style #8010

Closed

timgraham force-pushed the ticket_27656 branch 3 times, most recently from 1d78a25 to 3bbf8a1 Compare February 17, 2017 16:37

timgraham force-pushed the ticket_27656 branch 2 times, most recently from 3a1eb45 to adfe549 Compare February 21, 2017 17:07

timgraham mentioned this pull request Feb 23, 2017

WIP: Refs #27656 removing PEP warnings from code #8106

Closed

timgraham force-pushed the ticket_27656 branch from adfe549 to 93b3c71 Compare February 28, 2017 14:23

Refs #27656 -- Updated remaining docstring verbs according to PEP 257.

86de930

timgraham force-pushed the ticket_27656 branch from 93b3c71 to 86de930 Compare March 4, 2017 15:12

timgraham changed the title ~~Fixed #27656 -- Fix comment verb style according to PEP 257.~~ Fixed #27656 -- Updated docstring verbs according to PEP 257. Mar 4, 2017

timgraham merged commit 86de930 into django:master Mar 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #27656 -- Updated docstring verbs according to PEP 257. #7763

Fixed #27656 -- Updated docstring verbs according to PEP 257. #7763

desecho commented Dec 29, 2016 •

edited by timgraham

Loading

timgraham Dec 29, 2016

timgraham Dec 29, 2016

timgraham Dec 29, 2016

timgraham Dec 29, 2016

desecho commented Dec 29, 2016

timgraham commented Dec 29, 2016

desecho commented Dec 29, 2016

timgraham commented Dec 29, 2016

desecho commented Dec 29, 2016

timgraham Dec 29, 2016

timgraham Dec 29, 2016

timgraham Dec 29, 2016

timgraham Dec 29, 2016

timgraham Dec 29, 2016

timgraham Dec 29, 2016

timgraham commented Dec 30, 2016

desecho commented Dec 30, 2016

timgraham commented Dec 30, 2016

timgraham commented Dec 31, 2016

desecho commented Jan 21, 2017

timgraham commented Jan 24, 2017

desecho commented Jan 25, 2017

timgraham commented Feb 2, 2017

timgraham commented Feb 4, 2017

Fixed #27656 -- Updated docstring verbs according to PEP 257. #7763

Fixed #27656 -- Updated docstring verbs according to PEP 257. #7763

Conversation

desecho commented Dec 29, 2016 • edited by timgraham Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desecho commented Dec 29, 2016

timgraham commented Dec 29, 2016

desecho commented Dec 29, 2016

timgraham commented Dec 29, 2016

desecho commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timgraham commented Dec 30, 2016

desecho commented Dec 30, 2016

timgraham commented Dec 30, 2016

timgraham commented Dec 31, 2016

desecho commented Jan 21, 2017

timgraham commented Jan 24, 2017

desecho commented Jan 25, 2017

timgraham commented Feb 2, 2017

timgraham commented Feb 4, 2017

desecho commented Dec 29, 2016 •

edited by timgraham

Loading