Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

feat(checker): update D202 for inner functions #395

Closed
wants to merge 1 commit into from

Conversation

lewisacidic
Copy link

@lewisacidic lewisacidic commented Aug 5, 2019

Changes D202: No blank lines allowed after function docstring to allow
space below function docstrings with inner functions:

i.e. allows

def outer():
    """Valid docstring."""

    def inner():
        pass

    return pass

See comment from @cdeil in #361.

Thanks for submitting a PR!

Please make sure to check for the following items:

  • Add unit tests and integration tests where applicable.
    If you've added an error code or changed an error code behavior,
    you should probably add or change a test case file under tests/test_cases/ and add
    it to the list under tests/test_definitions.py.
    If you've added or changed a command line option,
    you should probably add or change a test in tests/test_integration.py.
  • Add a line to the release notes (docs/release_notes.rst) under "Current Development Version".
    Make sure to include the PR number after you open and get one.

Please don't get discouraged as it may take a while to get a review.

@lewisacidic
Copy link
Author

Dirty-looking change to fix the compatibility clash with black when it comes to inner functions.

I didn't take long to look through the library for a nicer way to do this, just thought I would get the ball rolling. Would be nice to have, as I have to disable D202 for our projects otherwise.

Changes D202: "No blank lines allowed after function docstring" to
allow space below function docstrings with inner functions.

i.e. allows:

```python

def outer():
    """Valid docstring."""

    def inner():
        pass

    return pass
```

See comment from @cdeil in PyCQA#361.

feat(checker): add test case to test_definitions

docs(changelog): add change to release notes
Copy link

@Code0x58 Code0x58 left a comment

Choose a reason for hiding this comment

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

👍 for taking the initiative

@@ -199,7 +199,9 @@ def check_no_blank_before(self, function, docstring): # def
if blanks_before_count != 0:
yield violations.D201(blanks_before_count)
if not all(blanks_after) and blanks_after_count != 0:
yield violations.D202(blanks_after_count)
def_follows = after.split("\n")[2].lstrip().startswith('def')
Copy link

@Code0x58 Code0x58 Sep 14, 2019

Choose a reason for hiding this comment

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

To allow both classes and functions and avoid including variables that have a name starting with def you could use:

re(r"\s+(?:class|def)\s").match(after)

This follows how python's re module is used in this file, where re is an alias for re.compile. It would be good to add a test for the class case too.

The check docstring could use updating to reflect the allowance of a single line if it is before a class/function definition. Same goes for the violation descriptions.

@johanfleury
Copy link
Contributor

@lewisacidic are you still working on this?

I see there's not much left to do (fix the docstring at the top of functions.py, use re as asked by @Code0x58), I can probably take it over if you can't (or don't want to) work on this PR anymore?

@lewisacidic
Copy link
Author

Oh sorry completely forgot about this - I'm really busy until end of next week. I could come back to it then, but feel free to finish off if you want!

btw sorry for brutalizing this with force pushes...

@johanfleury
Copy link
Contributor

johanfleury commented Oct 24, 2019

I created PR #426 with @Code0x58 recommendation.

peterjc added a commit to peterjc/biopython that referenced this pull request May 9, 2020
See psf/black#709 and
PyCQA/pydocstyle#395

pydocstyle v5.0.0 (and thus flake8 D202) was
relaxed to accept the blank lines added by black.
@peterjc
Copy link
Contributor

peterjc commented May 9, 2020

Can this be closed with #426 merged instead?

@samj1912 samj1912 closed this May 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants