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

Fix handling of dedented continuation lines. #472

Merged
merged 3 commits into from Aug 6, 2020
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 7, 2020

  • Dedent the function definition before parsing it.
  • Join continued-lines before attempting to parse the docstring.

Alternative to #441; further improves handling of parameter descriptions with continued lines (which were not handled correctly before).

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.

@samj1912
Copy link
Member

samj1912 commented May 8, 2020

Thanks for the patch. But from what I can see, this introduces new behaviour which is unrelated to the original issue. (It looks like if the doc string now has line continuation, it no longer errors with a D207). Although this might be a good improvement, can we please file a related issue to the repo with this behaviour and reference that in this PR instead?

Also it looks like most of the code in this PR is taken from #441

Can you either pick my commits from that PR to preserve the original code attribution or wait for it to be merged and create a new PR with the improved handling of line continuations?

I would prefer the latter.

@samj1912
Copy link
Member

samj1912 commented May 8, 2020

Looks like the issue this PR is solving is - #234

Let's update the PR, description and docs to match the same.

@anntzer
Copy link
Contributor Author

anntzer commented May 8, 2020

The only reason I opened this separately is because you didn't pick up #441 (comment). Now that you did I have no problem waiting for your PR to go in first and rebase this one over yours.

@samj1912
Copy link
Member

samj1912 commented Jul 8, 2020

The original PR was fixed. Would you like to rebase and update the description with the dedented bug fix?

@samj1912
Copy link
Member

@anntzer bump?

@samj1912 samj1912 self-requested a review July 15, 2020 19:30
@anntzer anntzer force-pushed the indent branch 2 times, most recently from c91c202 to 8e947e8 Compare July 20, 2020 13:32
@anntzer
Copy link
Contributor Author

anntzer commented Jul 20, 2020

fixed :)

Copy link
Member

@samj1912 samj1912 left a comment

Choose a reason for hiding this comment

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

Thanks for this. Sorry for the late review. Can you also add a test that tests for line continuation in sections beside the Params one?

@anntzer
Copy link
Contributor Author

anntzer commented Aug 4, 2020

What do you actually want to test for?

@samj1912
Copy link
Member

samj1912 commented Aug 4, 2020

That it doesn't throw D207 for normal doc string descriptions with line continuations

- Dedent the function definition before parsing it.
- Join continued-lines before attempting to parse the docstring.
@anntzer
Copy link
Contributor Author

anntzer commented Aug 4, 2020

done

samj1912
samj1912 previously approved these changes Aug 5, 2020
@@ -20,6 +20,8 @@ Bug Fixes
The bug caused some argument names to go unreported in D417 (#448).
* Fixed an issue where skipping errors on module level docstring via #noqa
failed when there where more prior comments (#446).
* Support backslash-continued parameter descriptions in Numpy-style docstrings
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it doing this for more than just numpy parameter descriptions at this point?

Copy link
Member

Choose a reason for hiding this comment

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

should we update this?

@samj1912 samj1912 merged commit b0f7d62 into PyCQA:master Aug 6, 2020
@anntzer
Copy link
Contributor Author

anntzer commented Aug 6, 2020

thanks for fixing the last part :)

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.

None yet

2 participants