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

Fix #309 Missing helptext for multiline argument #335

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

xosnos
Copy link

@xosnos xosnos commented Apr 24, 2021

Closes #309

Having colons on wrapped lines no longer causes missing help-text for multiline arguments, as long as the user follows the Google Python Style Guide for docstring args: use a hanging indent of 2 or 4 spaces more than the parameter name.

@MichaelCG8 can you review this? Thanks :)

@google-cla google-cla bot added the cla: yes Author has signed CLA label Apr 24, 2021
@MichaelCG8
Copy link
Contributor

Nice one!

Got a couple of thoughts.

  • There's a pre-existing comment in _consumer_google_args_line() that says # first is either the "arg" or "arg (type)". Could you update that now that we know it could be other things?
  • This logic for new arguments: new_arg = line_info.indentation <= line_info.previous.indentation looks like it might be True when there's more than one indented line for the argument description - the second line will have the same indentation as the previous. I haven't checked this though so I might have missed something :) If there is a problem, you could possibly compare to the indentation of the first line for that argument (this might already be available somewhere).
  • In _as_arg_name_and_type() the line if _is_arg_name(tokens[0]) and not _is_arg_name(tokens[1]):. This will work if the line is something like "arg (type)" but what about a line like "arg - here is an arg"? Then tokens[1] will be "-". The intention here is to match anything where tokens[1] starts and ends with one of the pairs "()", "[]", "{}". Maybe a new function _is_type_token() would be worthwhile for this task.

Great to see that you've added tests too! In your first one, I think you need to indent the second line of description for param2. It might also be worth adding one where the description is over 3 lines, with the colon in the last one. That should catch the line_info.indentation issue I mentioned above (if that is a real issue ;) )

- Update comments in _consume_google_args_line to match new logic
- Fix indentation logic: indent_check compares current line indent with first line indent for argument.
- Add new function _is_arg_type to check if arg type is of valid form
- Fix tests for multiline helptexts with colons and indents
@xosnos
Copy link
Author

xosnos commented Apr 26, 2021

Thanks for the review @MichaelCG8
I've made those changes you suggested.

@MichaelCG8
Copy link
Contributor

Looks great! I think this can be merged.

One minor thing that you may or may not wish to address is that your new function _is_arg_type() will actually return None if none of the regexes match, and if one of them does then it will return that re.Match object. This is because the or operator returns the first truthy value it encounters, and if all operands are falsy it returns the last one:

a={}; b=[1]; c=None
a or b or c
# [1]

a={}; b=[]; c=None
a or b or c
# None

If you want it to strictly return True or False you can wrap it in a call to bool(). Or you could update the docstring to say it returns "A re.Match object if type_ looks like an arg type, None otherwise."`

Logically though, what you've done works fine.

I believe @dbieber and @joejoevictor are the folk with merging powers, and they sometimes make stylistic changes when merging so you could leave as is and they'll make that change if they feel it's necessary.

Thanks for doing the work!

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.

Missing helptext for multiline argument.
2 participants