Skip to content
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

gh-104683: Argument Clinic: Refactor and simplify 'add docstring' states #107550

Merged
merged 3 commits into from
Aug 1, 2023

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 1, 2023

Introduce docstring_append() helper, and use it for both parameter and
function docstrings. Remove docstring fixup from
do_post_block_processing_cleanup(); instead, make sure no fixup is needed.

…g' states

Introduce docstring_append() helper, and use it for both parameter and
function docstrings. Remove docstring fixup from
do_post_block_processing_cleanup(); instead, make sure no fixup is needed.
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Tools/clinic/clinic.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Remove docstring fixup from
do_post_block_processing_cleanup(); instead, make sure no fixup is needed.

I think the fixup is still needed. If I apply this diff to your PR branch:

--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -5577,6 +5577,11 @@ def do_post_block_processing_cleanup(self) -> None:
             if no_parameter_after_star:
                 fail("Function " + self.function.name + " specifies '*' without any parameters afterwards.")

+        for name, value in self.function.parameters.items():
+            if not value:
+                continue
+            assert value.docstring == value.docstring.rstrip()
+
         self.function.docstring = self.format_docstring()

Then the assertion fails in ClinicParserTest.test_function_docstring:

======================================================================
FAIL: test_function_docstring (test.test_clinic.ClinicParserTest.test_function_docstring)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\alexw\coding\cpython\Lib\test\test_clinic.py", line 632, in test_function_docstring
    function = self.parse_function("""
               ^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\cpython\Lib\test\test_clinic.py", line 1384, in parse_function
    block = self.parse(text)
            ^^^^^^^^^^^^^^^^
  File "C:\Users\alexw\coding\cpython\Lib\test\test_clinic.py", line 1380, in parse
    parser.parse(block)
  File "C:\Users\alexw\coding\cpython\Tools\clinic\clinic.py", line 4612, in parse
    self.do_post_block_processing_cleanup()
  File "C:\Users\alexw\coding\cpython\Tools\clinic\clinic.py", line 5583, in do_post_block_processing_cleanup
    assert value.docstring == value.docstring.rstrip()
AssertionError

----------------------------------------------------------------------

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@erlend-aasland
Copy link
Contributor Author

I think the fixup is still needed. [...]

The docstrings are further processed in format_docstring; I'm not sure it matters that they are off at this point in the parsing.

@AlexWaygood
Copy link
Member

I think the fixup is still needed. [...]

The docstrings are further processed in format_docstring; I'm not sure it matters that they are off at this point in the parsing.

Ugh, yes, I think you're right :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good now the test's been added :)

@erlend-aasland
Copy link
Contributor Author

Ugh, yes, I think you're right :)

See also the added test ;)

@erlend-aasland erlend-aasland enabled auto-merge (squash) August 1, 2023 22:58
@erlend-aasland erlend-aasland merged commit b4d8897 into python:main Aug 1, 2023
17 of 18 checks passed
@erlend-aasland erlend-aasland deleted the clinic/docstring-append branch August 22, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants