parsing function help with multiline

[hermanzhaozzzz]

Can I have flexible line breaks in the help comments?

this is the help doc

def func(input_table: str):
        """Plot region mutation info using table generated by `bioat bam mpileup_to_table`.

        :param input_table: Path of table generated by `bioat bam mpileup_to_table`.
        This table should contain base mutation information from a short genome region (no more than 1k nt).

printed help I got

POSITIONAL ARGUMENTS
    INPUT_TABLE
        Type: str
        Path of table generated by `bioat bam mpileup_to_table`. This table should contain base mutation information from a short genome region (no more than 1k nt).

printed help I expacted

POSITIONAL ARGUMENTS
    INPUT_TABLE
        Type: str
        Path of table generated by `bioat bam mpileup_to_table`. 
        This table should contain base mutation information from a short genome region (no more than 1k nt).

[dbieber]

The docstring parsing code is in https://github.com/google/python-fire/blob/master/fire/docstrings.py

Multiline rst args should be handled by

python-fire/fire/docstrings.py

Lines 468 to 469 in d44d33d

	elif state.section.format == Formats.RST:
	state.current_arg.description.lines.append(line_info.remaining.strip())

You may have identified a bug.

[thebadcoder96]

I want to work on this. First time contributor here.

Was looking into the docstrings.py file and I think the problem is with _join_lines(). I kinda made a quick fix but I will dive deeper. Some guidance would be awesome!

https://github.com/thebadcoder96/python-fire-os/blob/45e3a0e7be335b33fd83b24b06ba8fdc1baee884/fire/docstrings.py#L271-L278

What do you guys think?

[dbieber]

The first step would be to add one or more failing test cases, e.g. inspired from the comment above #448 (comment)
Then if you see the issue and can fix it, that would be a welcome change!

The test cases would go in https://github.com/google/python-fire/blob/master/fire/docstrings_test.py

---

Notice at line 253 there's "# TODO(dbieber): Add parameters for variations in whitespace handling."
I don't have this code loaded into memory -- do you happen to see what the issue would be with always using \n to join lines? I wonder why I wanted multiple variations for whitespace handling from the beginning.

One possibility that comes to mind is that the first line might be shorter than subsequent lines because it includes the arg name as a prefix, and so we'd get weird formatting if we were to simply keep all the newlines as is (but strip that prefix).

But other times, keeping newlines is key, e.g. if there's ascii art in the docstring or other spacing-specific content.

[thebadcoder96]

I recreated the issue on my end and then resolved it with the code I mentioned in the comment above . I don't think this is the real fix rather a quick fix.

do you happen to see what the issue would be with always using \n to join lines?

I was thinking along the same lines as you were and I was using \n to join lines always. But when I ran the docstring tests, 3 of them failed. The tests were test_google_format_multiline_arg_description, test_numpy_format_multiline_arg_description, and test_numpy_format_typed_args_and_returns.

Here is one of the failed tests:

======================================================================
FAIL: test_numpy_format_multiline_arg_description (__main__.DocstringsTest.test_numpy_format_multiline_arg_description)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mishals/Documents/GitHub Open Source Repos/python-fire-os/fire/docstrings_test.py", line 263, in test_numpy_format_multiline_arg_description
    self.assertEqual(expected_docstring_info, docstring_info)
AssertionError: Docst[314 chars]o cover two lines.')], returns=None, yields=None, raises=None) != Docst[314 chars]o cover two\nlines.')], returns=None, yields=None, raises=None)

I think that using \n to join always is a good use. In fact, I would argue that the test is wrong. The test docstring is

Docstring summary.

        This is a longer description of the docstring. It spans across multiple
        lines.

        Parameters
        ----------
        param1 : int
            The first parameter.
        param2 : str
            The second parameter. This has a lot of text, enough to cover two
            lines.

But the test expects this output:

NAME
    test.py - Docstring summary.

SYNOPSIS
    test.py PARAM1 PARAM2

DESCRIPTION
    This is a longer description of the docstring. It spans across multiple
    lines.

POSITIONAL ARGUMENTS
    PARAM1
        Type: int
        The first parameter.
    PARAM2
        Type: str
        The second parameter. This has a lot of text, enough to cover two lines.

NOTES
    You can also use flags syntax for POSITIONAL ARGUMENTS

Notice how the second parameter does not split into the next line?

If we use \n to join, in my opinion, it would bring in the new line like how it is in the docstring. I tried looking into it further and found that when we do not use \n, the lines are stripped and stored as one long string. If we do use line breaks, then the code would split it into 2 or more different strings/lines which while joining back should use \n.

I was running late working on it last night and wanted to update you guys with something so I made the quick fix. I think the best way to go about it would be to always use \n to join lines like you said and edit the 3 test cases to pass only when the new line is present. Also, I could add the test case for this issue as you mentioned.

I am not 100% sure since I do not know all the intricacies of the docstring and also this is my first time contributing ever. What do you think of everything mentioned above? also can you assign this issue to me?

[thebadcoder96]

Btw, the other 2 tests that failed also fail the same way.

[dbieber]

I agree that it would be OK to adjust the behavior of those 3 tests.

I do have one concern about simply always using \n, and that's that it might lead to poor formatting in some simple cases.
In particular suppose (1) the arg name is long, (2) the arg description starts on the same line as the arg name, and (3) the arg description spans a few lines. Then the first line of the extracted arg description might be really short.

Maybe this is okay. I wonder if there's an approach that gets the best of both approaches though. Wdyt?

[thebadcoder96]

I will work on updating the tests. As far as your concerns, this is how I think it would work; Please correct me if I am wrong.

(1) the arg name is long, (2) the arg description starts on the same line as the arg name,

Right now, the code extracts the docstring and splits by \n. so if the arg name is long or the description starts on the same line, it would be extracted as a really long string. And when we are printing it out, it would print out that long string which would preserve the original docstring. So if we are joining the lines always by \n this would not change since the line is just one huge string.

and (3) the arg description spans a few lines.

in this case, the docstring is extracted and split by \n so every line is a string inside a list. the code right now would join them using ' ' which does not preserve the original docstring. joining by \n would create those line breaks.

I may be missing something. But I think that \n always is the way to go here and we would not have any issues.

I wonder if there's an approach that gets the best of both approaches though. Wdyt?

if you plan to expand your docstring capabilities to handle more nuances, I think one way to go about it is what I did for the quick fix. I did it initially to pass the docstring test but I think we can use it. :P

I created a new parameter for _join_lines() called type (bad name) that would possibility label the what part of docstring (type of lines) needs to be joined. We can then accordingly add more functionality as the need arises. What do you think?

[dbieber]

so if the arg name is long or the description starts on the same line, it would be extracted as a really long string.

See the part of the logic where line_info.remaining is set. If I'm remembering right, this will exclude the arg name, leaving just the part of the line that follows. If the arg name is long, the remaining part of the line will be quite short.

Edit: ignore this comment. line_info.remaining is still logically a whole line. I thought it was only the part after the arg, but I was wrong. The part after the arg is named "second" in the code e.g. at line 397 as linked in the next comment, and (from a cursory glance) seems to only apply to the Google docstring format.

[dbieber]

It looks like for the RST format, whole lines are used are used so my concern would not arise.

For the Google format, the concern I have arises at

python-fire/fire/docstrings.py

Line 397 in d44d33d

arg.description.lines.append(second.strip())

For numpy, if I'm thinking about this correctly, whole lines are used so my concern would not arise.

[thebadcoder96]

Can you give me an example of where your concern is happening? I'm slightly confused about the part where you said it would arise. I tried to use the google docstring test string to test it out and tried some of my variations, making the string long and spanning multiple lines, but it seems to work.

Please review the changes proposed in the PR below. if you can give me a test case where your concern will be caused, it will help me understand it better and fix it.

[dbieber]

👍 Here's an example:

def make_function_maker_handler_event(function_maker_handler_event_factory):
  """This is a Google-style docstring with long args.

  Args:
    function_maker_handler_event_factory: The function-maker-handler event factory
      responsible for making the function-maker-handler event. Use this whenever
      you need a function-maker-handler event made for you programmatically.
  """

I haven't double-checked this, but I expect if we use the \n approach this will show up in the help as:

The function-maker-handler event factory
responsible for making the function-maker-handler event. Use this whenever
you need a function-maker-handler event made for you programmatically.

with the first line awkwardly short.