python-fire
python-fire copied to clipboard
Fix #309 Missing helptext for multiline argument
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 :)
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 beTrue
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 lineif _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"
? Thentokens[1]
will be"-"
. The intention here is to match anything wheretokens[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 ;) )
Thanks for the review @MichaelCG8 I've made those changes you suggested.
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!