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

issue-261 Fixed missing parts of argument descriptions #262

Conversation

MichaelCG8
Copy link
Contributor

issue-261 Fixed missing parts of argument descriptions for Google and Numpy style docstrings.

@dbieber
Copy link
Member

dbieber commented Dec 16, 2020

Belated thanks for this PR!

If I'm understanding correctly:

There are two behavior changes within:

  1. We now use arg_pattern = r'^[a-zA-Z_]\w*$' for determining valid argument names.
  2. We now set state.current_arg = arg when we encounter a Google arg-and-type.

Change 1. doesn't seem so important -- it prevents the parser from treating some invalid arg names as args, but as I understand it this increased strictness hasn't come up in practice.

Change 2. is important; it fixes a bug where previously we were losing some information about multiline arg descriptions.

Can you confirm my understanding is correct?

Thanks.

Thanks also for the test cases 👍

@MichaelCG8
Copy link
Contributor Author

With similar belatedness, thank you for reviewing!

Change 1
The old code would treat a line as an arg name if it is one word long and doesn't contain a :. The case that fails is any multiline description where the last line consists of one word only, as in foo3 #261 (comment)

This could have been fixed by also checking for a . in the string, so that any text ended with a period would be parsed as the plaintext description. The regex is more general and fixes the behaviour for any string that isn't a valid argument pattern, for example if the docstring ends with a reference to a type by surrounding it in back ticks.

The case where there is a single word followed by a period is something I've hit a few times if I reach my line length limit and wrap the last word onto a new line.

Change 2
Spot on.

python-fire-bot pushed a commit that referenced this pull request Jan 22, 2021
… Numpy style docstrings.

482bb9d by Michael Garbutt <[email protected]>:

COPYBARA_INTEGRATE_REVIEW=#262 from MichaelCG8:issue-261-bugfix-multi-line-docstring-parameter-descriptions 482bb9d
PiperOrigin-RevId: 353248737
Change-Id: I3033200c69c5aba0ce9bb819574f37248e32c1cb
@dbieber
Copy link
Member

dbieber commented Jan 22, 2021

Closed with c39de6a

@dbieber dbieber closed this Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Author has signed CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants