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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: fix formatting of docstring with lists #1687

Merged
merged 13 commits into from
Jul 7, 2023
Merged

Conversation

parthea
Copy link
Contributor

@parthea parthea commented Jul 6, 2023

Fixes #1345 馃

The PR applies the following fixes for the function that converts documentation to RST format.

  • Each item in a list should have a new line
  • The new line after a colon in a list should be preserved
  • When new lines wrap in a list, they should be indented.

All of the changes have corresponding tests to show the behaviour without the fixes.

@parthea parthea changed the title add test to show issue with lists docs: fix formatting of docstring with lists Jul 6, 2023
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jul 7, 2023
@parthea parthea marked this pull request as ready for review July 7, 2023 14:28
@parthea parthea requested review from a team as code owners July 7, 2023 14:28
@@ -115,7 +123,9 @@ def wrap(text: str, width: int, *, offset: Optional[int] = None, indent: int = 0
text='\n'.join([textwrap.fill(
break_long_words=False,
initial_indent=' ' * indent,
subsequent_indent=' ' * indent,
# ensure that subsequent lines for lists are indented 2 spaces
subsequent_indent=' ' * indent + \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add spaces around =.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried making this change but the style check failed

(py39) partheniou@partheniou-vm-3:~/git/gapic-generator-python$ find gapic tests -name "*.py" -not -path 'tests/**/goldens/*' | xargs autopep8 --diff --exit-code
--- original/gapic/utils/lines.py
+++ fixed/gapic/utils/lines.py
@@ -124,7 +124,7 @@
             break_long_words=False,
             initial_indent=' ' * indent,
             # ensure that subsequent lines for lists are indented 2 spaces
-            subsequent_indent = ' ' * indent + \
+            subsequent_indent=' ' * indent + \
             ('  ' if token.strip().startswith('-') else ''),
             text=token,
             width=width,

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if enclosing the right side in parentheses, or breaking this down into if-else statements would work.

# as the new line is required for lists.
if '\n' in text:
initial_text = text.split('\n')[0]
if ":" not in initial_text:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could : appear on text line 2, and the list starts from line 3 and on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I added a test case in e08978a

@parthea parthea enabled auto-merge (squash) July 7, 2023 16:59
@parthea parthea merged commit abe0f3f into main Jul 7, 2023
51 checks passed
@parthea parthea deleted the ensure-new-line-lists branch July 7, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sphinx warning in generated client for google/cloud/gkebackup/v1
2 participants