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

Fix indentation error while parsing class methods #441

Merged
merged 2 commits into from
Jul 8, 2020

Conversation

sambhav
Copy link
Member

@sambhav sambhav commented Dec 15, 2019

In order to determine the missing arguments, we currently
use ast.parse to parse partial source code. This parsing
might lead to syntax errors. We catch the syntax error and
make the parsing more resilient to errors in the source code.

Fixes #437

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.

@sambhav
Copy link
Member Author

sambhav commented Dec 15, 2019

@Nurdok review please :)

@sambhav sambhav force-pushed the indent branch 2 times, most recently from 5c4a69b to aff6360 Compare January 10, 2020 22:01
@sambhav
Copy link
Member Author

sambhav commented Jan 10, 2020

@Nurdok bump?

@sambhav
Copy link
Member Author

sambhav commented Jan 25, 2020

@Nurdok bump

1 similar comment
@sambhav
Copy link
Member Author

sambhav commented Feb 29, 2020

@Nurdok bump

@tacaswell
Copy link

I don't think the test actually tests the issue in the original issue.

Because we need to put the full parameter specification on one line to make sphinx happy we use line continuation to wrap it, but if you indent the continuation, you end up with a whole bunch of spaces in the line.

I also do not agree that this is "incorrect" indentation for the same reason.

@sambhav
Copy link
Member Author

sambhav commented Apr 25, 2020

@Nurdok bump

Nurdok
Nurdok previously approved these changes May 5, 2020
@sambhav
Copy link
Member Author

sambhav commented May 5, 2020

I don't think the test actually tests the issue in the original issue.

Because we need to put the full parameter specification on one line to make sphinx happy we use line continuation to wrap it, but if you indent the continuation, you end up with a whole bunch of spaces in the line.

I also do not agree that this is "incorrect" indentation for the same reason.

The issue was caused because of "incorrectly" indented conent like #437 (comment). The parameters case you mention is a subset and is covered by this.

@tacaswell
Copy link

But the original issue (#437 (comment)) is correct. While I see why from a pure-parsing point of view these maybe the same, from a semantic point of view they are very different as with the \ line continuation the second line should not be "indented".

Can you please add a test of a "correctly" formatted docstring that uses line continuation as well?

@sambhav
Copy link
Member Author

sambhav commented May 8, 2020

But the original issue (#437 (comment)) is correct. While I see why from a pure-parsing point of view these maybe the same, from a semantic point of view they are very different as with the \ line continuation the second line should not be "indented".

Can you please add a test of a "correctly" formatted docstring that uses line continuation as well?

Added a test. It works as expected.

@sambhav
Copy link
Member Author

sambhav commented May 8, 2020

@Nurdok all comments should be addressed now. :)

In order to determine the missing arguments, we currently
use ast.parse to parse partial source code. This parsing
might lead to syntax errors. We catch the syntax error and
make the parsing more resilient to errors in the source code.

Fixes PyCQA#437
@sambhav
Copy link
Member Author

sambhav commented May 24, 2020

@Nurdok bump

Nurdok
Nurdok previously approved these changes May 28, 2020
@tacaswell
Copy link

Where we actually have to use line continuation in practice is in the type specification not in the body of the parameter description. A real life example: https://github.com/matplotlib/matplotlib/blob/5445d77460f71d16dd3e1b01c4c116a741baf6b9/lib/matplotlib/axes/_axes.py#L4392-L4402

class A:
    def standin(...):
        """
        ...

        Parameters
        ----------

        ...


        edgecolors : {'face', 'none', *None*} or color or sequence of color, \
default: :rc:`scatter.edgecolors`
            The edge color of the marker. Possible values:
            - 'face': The edge color will always be the same as the face color.
            - 'none': No patch boundary will be drawn.
            - A color or sequence of colors.
            For non-filled markers, the *edgecolors* kwarg is ignored and
            forced to 'face' internally.

        ...

        """
        ...

@sambhav
Copy link
Member Author

sambhav commented May 31, 2020

@tacaswell, I agree but this is unrelated to the original bug that this PR was made to solve. This has been a long standing issue long before #437 was reported. @anntzer has a fix for the line continuation bug at #472

Please see our conversation there for more details. Once this PR is merged, we will merge @anntzer 's PR with this fix.

@sambhav
Copy link
Member Author

sambhav commented Jul 5, 2020

@Nurdok I can't merge because the release notes merge conflict dismissed your review. Can you approve again please?

@sambhav sambhav merged commit dd5ab9e into PyCQA:master Jul 8, 2020
@sambhav sambhav deleted the indent branch July 8, 2020 08:20
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.

IndentationError on line continuations for numpydoc
4 participants