python-fire icon indicating copy to clipboard operation
python-fire copied to clipboard

Fix #309 Missing helptext for multiline argument

Open xosnos opened this issue 3 years ago • 3 comments

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 :)

xosnos avatar Apr 24 '21 20:04 xosnos

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 ;) )

MichaelCG8 avatar Apr 25 '21 09:04 MichaelCG8

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

xosnos avatar Apr 26 '21 17:04 xosnos

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!

MichaelCG8 avatar Apr 27 '21 21:04 MichaelCG8