Skip to content

CLN: D213: Multi-line docstring summary should start at the second line #31893

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

Conversation

simonjayhawkins
Copy link
Member

No description provided.

@simonjayhawkins simonjayhawkins changed the title D213: Multi-line docstring summary should start at the second line CLN: D213: Multi-line docstring summary should start at the second line Feb 11, 2020
@@ -86,7 +86,8 @@ class IndexingError(Exception):


class IndexingMixin:
"""Mixin for adding .loc/.iloc/.at/.iat to Datafames and Series.
"""
Mixin for adding .loc/.iloc/.at/.iat to Datafames and Series.
"""
Copy link
Member

Choose a reason for hiding this comment

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

do we have a policy on whether this should be one line or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

there is D200 One-line docstring should fit on one line with quotes, see http://www.pydocstyle.org/en/5.0.2/error_codes.html, but I think that would go against the code cleanups in #31162, #31462 and others.

we want consistency. so we need a policy.

Copy link
Member Author

Choose a reason for hiding this comment

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

there are ~550 one-line docstrings split over 3 lines.

@@ -74,7 +74,8 @@ def is_url(url) -> bool:
def _expand_user(
filepath_or_buffer: FilePathOrBuffer[AnyStr],
) -> FilePathOrBuffer[AnyStr]:
"""Return the argument with an initial component of ~ or ~user
"""
Return the argument with an initial component of ~ or ~user
replaced by that user's home directory.
Copy link
Member

Choose a reason for hiding this comment

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

this introduces a weird alignment here

Copy link
Member Author

Choose a reason for hiding this comment

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

this PR is just D213, the alignment will be fixed with D207/D208

Copy link
Member Author

Choose a reason for hiding this comment

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

so some overlap here with #31890, and D207/D208 will need to be rerun after D209/D213 merged

Copy link
Member Author

Choose a reason for hiding this comment

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

i'll push the fix in this PR. (prob thur)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@jbrockmendel
Copy link
Member

Mostly looks fine, only question is if we want to avoid introducing alignment mismatches

@WillAyd
Copy link
Member

WillAyd commented Feb 11, 2020

Can you merge master?

@@ -755,7 +756,8 @@ def _value_with_fmt(self, val):

@classmethod
def check_extension(cls, ext):
"""checks that path's extension against the Writer's supported
"""
checks that path's extension against the Writer's supported
extensions. If it isn't supported, raises UnsupportedFiletypeError."""
Copy link
Member

Choose a reason for hiding this comment

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

Since you are changing things here, can you put the ending triple quotes on their own line as well? (other cases below as well)

Copy link
Member Author

Choose a reason for hiding this comment

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

see #31891

Copy link
Member Author

Choose a reason for hiding this comment

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

probably best to merge #31891 first, since less violations there and once merged this diff should look good and any leftover misalignment can be pushed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

…docstring-summary-should-start-at-the-second-line
@@ -74,7 +74,8 @@ def is_url(url) -> bool:
def _expand_user(
filepath_or_buffer: FilePathOrBuffer[AnyStr],
) -> FilePathOrBuffer[AnyStr]:
"""Return the argument with an initial component of ~ or ~user
"""
Return the argument with an initial component of ~ or ~user
replaced by that user's home directory.
Copy link
Member Author

Choose a reason for hiding this comment

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

i'll push the fix in this PR. (prob thur)

cacher.
"""
self._cacher = (item, weakref.ref(cacher))

def _reset_cacher(self) -> None:
"""Reset the cacher."""
"""
Reset the cacher."""
Copy link
Member Author

Choose a reason for hiding this comment

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

i'll fix in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Feb 12, 2020
…docstring-summary-should-start-at-the-second-line
@simonjayhawkins simonjayhawkins merged commit cc4c0b3 into pandas-dev:master Feb 15, 2020
@simonjayhawkins simonjayhawkins deleted the D213--Multi-line-docstring-summary-should-start-at-the-second-line branch February 15, 2020 20:15
roberthdevries pushed a commit to roberthdevries/pandas that referenced this pull request Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants