Docstring description multiline parsing #476

thebadcoder96 · 2023-12-23T04:32:03Z

this would potentially close #448

Please review.

fire/docstrings.py

fire/docstrings_test.py

fire/docstrings.py

thebadcoder96 · 2024-01-14T01:53:24Z

Sorry I was slacking off a bit :P

I have found another issue with the code (a new possible test case). When I use : in the second or third line, the line disappears since it is detected as an arg with the current logic. I also found that my line3 logic to check if it was an arg was wrong. Below is the example for the : issue.

Ex: if I had something like this:

    param3_that_is_very_longer_than_usual: The second parameter. This has a lot of text,
      enough to: cover two lines.

it would print out:

PARAM3_THAT_IS_VERY_LONGER_THAN_USUAL
        The second parameter. This has a lot of text,

The problem is with _as_arg_name_and_type() because when the part split by : passes through it, it only checks if there is more than 1 string split by ' ' and if the first word looks like an arg, combines everything else into a string removing ()[]{} (out current logic). I have added another condition to check if these brackets are present to qualify the whole string as an arg and type in that function.

When I made this change I noticed that around line 408/417 within _consume_google_args_line()

      else:
        if state.current_arg:
          state.current_arg.description.lines.append(split_line[0])

Here we are appending only split_line[0] but we should be joining everything back with : and then appending it. I made this change as well. Then I realized that we never do the check for line2 and line3 and store their values (arg states) here so if there is a line with : that is not an arg it does not check for the code I have added.

I decided to convert the check into a function and call them in both places when the line is not an arg. Added 3 more tests regarding the :

dbieber · 2024-01-14T05:22:20Z

Ack. That all sounds good. Will update you when I get to the review.

…

On Sat, Jan 13, 2024 at 9:57 PM Mishahal Palakuniyil < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In fire/docstrings.py <#476 (comment)>: > @@ -410,6 +419,83 @@ def _consume_google_args_line(line_info, state): else: if state.current_arg: state.current_arg.description.lines.append(split_line[0]) + state.max_line_length = max(state.max_line_length, len(line_info.line)) + if line_info.previous.line == state.line1: # check for line2 I have added the index as you guided and updated the code to compare indexes instead of strings. Please review the most recent commit. — Reply to this email directly, view it on GitHub <#476 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGZ6XKTW2ZEX7I5P7DXH4TYONCTHAVCNFSM6AAAAABBAQ6XUKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMRQGEZTKOBUGM> . You are receiving this because you commented.Message ID: ***@***.***>

thebadcoder96 · 2024-01-14T16:12:54Z

Ack. That all sounds good. Will update you when I get to the review.

What I mentioned might be another bug. I also just thought about this; I was messing around adding : in the second and third lines of the description. I always added it after 2 words (like in the example mentioned above) which caused me to believe _as_arg_name_and_type was the problem.

I haven't tested this, but if we add : after the first word in descriptions (line2 or later), the _is_arg_name() function will be called and according to our current logic, anything that is just a word will be considered as an arg. I think we want a better way to identify an arg.

On the top of my head, I'm thinking we can do something like compare the previous line if state.current_arg exists and see if there is a difference in indentation (most likely not an arg if line2 first word and arg name indentation difference is present). Just spit-balling my thoughts right now.

jamesowers-cohere · 2024-02-08T15:52:22Z

fire/docstrings_test.py

+  def test_google_format_long_arg_long_description(self):
+    docstring = """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. 
+    """
+    docstring_info = docstrings.parse(docstring)
+    expected_docstring_info = DocstringInfo(
+        summary='This is a Google-style docstring with long args.',
+        args=[
+            ArgInfo(name='function_maker_handler_event_factory',
+                    description='The function-maker-handler event factory '
+        'responsible for making the function-maker-handler event. '
+        'Use this whenever\nyou need a function-maker-handler event'
+         ' made for you programmatically.'),
+        ],
+    )


It may be good to add an example here, or below, which contains a second line in the documentation before the args (since this is very common). e.g.

Suggested change

def test_google_format_long_arg_long_description(self):

docstring = """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.

"""

docstring_info = docstrings.parse(docstring)

expected_docstring_info = DocstringInfo(

summary='This is a Google-style docstring with long args.',

args=[

ArgInfo(name='function_maker_handler_event_factory',

description='The function-maker-handler event factory '

'responsible for making the function-maker-handler event. '

'Use this whenever\nyou need a function-maker-handler event'

' made for you programmatically.'),

],

)

docstring = """This is a Google-style docstring with long args.

Here is some further and potentially long-winded explanation of the function.

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.

"""

docstring_info = docstrings.parse(docstring)

expected_docstring_info = DocstringInfo(

summary='This is a Google-style docstring with long args.\n\nHere is some further and potentially long-winded explanation of the function.',

args=[

ArgInfo(name='function_maker_handler_event_factory',

description='The function-maker-handler event factory '

'responsible for making the function-maker-handler event. '

'Use this whenever\nyou need a function-maker-handler event'

' made for you programmatically.'),

],

)

I like it. I think this test case would fail rn since the default behavior is to ignore the \n and just display without them.

This was the issue I fixed but I think I have fixed it within the scope of args but not the name of the function and summary. I will work on this as well.

Another thing I noticed was that the name of the function is displayed as <name> - <summary>. Is that how it should be? I only ask since we have a section called description where we display the summary.

Another thing I noticed was that the name of the function is displayed as <name> - <summary>.

I think so, though maybe there are examples where it doesn't make sense and we can improve it. I'd have to give that some thought, but fortunately that's orthogonal to this change.

dbieber · 2024-02-24T19:01:09Z

fire/docstrings.py

@@ -336,9 +342,10 @@ def _as_arg_name_and_type(text):
    None otherwise.
  """
  tokens = text.split()
+  is_type = any(c in "[](){}" for c in text)


What was the reason for this line?

I haven't tested this, but if we add : after the first word in descriptions (line2 or later), the _is_arg_name() function will be called and according to our current logic, anything that is just a word will be considered as an arg. I think we want a better way to identify an arg.

This was the initial reason for this. I just came up with this for the time being but i think that we need a better way to identify an arg name.

thebadcoder96 added 4 commits December 20, 2023 23:16

quick fix

45e3a0e

Added '\n' to _join_lines() and updated tests

b395e20

cleaned py file

6f0ce18

Added changes based on discussion

4b770d9

thebadcoder96 mentioned this pull request Jan 2, 2024

parsing function help with multiline #448

Open

dbieber reviewed Jan 2, 2024

View reviewed changes

fire/docstrings.py Outdated Show resolved Hide resolved

dbieber reviewed Jan 2, 2024

View reviewed changes

fire/docstrings_test.py Show resolved Hide resolved

thebadcoder96 added 4 commits January 2, 2024 13:23

updated code to resolved comment

93ff111

Changed threshold to 40%

645c51d

Merge branch 'google:master' into docstring-description-parsing

eb8694f

Added test cases

c56d951

dbieber reviewed Jan 9, 2024

View reviewed changes

fire/docstrings.py Outdated Show resolved Hide resolved

fire/docstrings.py Outdated Show resolved Hide resolved

thebadcoder96 added 3 commits January 13, 2024 18:02

Updated code to per-arg state

446065d

Updated to per-arg state

2458562

made code more robust and added test cases (handling :)

5ce7726

Updated sttring comparison to index

794f48e

jamesowers-cohere reviewed Feb 8, 2024

View reviewed changes

dbieber reviewed Feb 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Docstring description multiline parsing #476

Docstring description multiline parsing #476

thebadcoder96 commented Dec 23, 2023

thebadcoder96 commented Jan 14, 2024 •

edited

Loading

dbieber commented Jan 14, 2024 via email

thebadcoder96 commented Jan 14, 2024

jamesowers-cohere Feb 8, 2024

thebadcoder96 Feb 9, 2024

thebadcoder96 Feb 9, 2024 •

edited

Loading

dbieber Feb 24, 2024 •

edited

Loading

dbieber Feb 24, 2024

thebadcoder96 Feb 26, 2024 •

edited

Loading

Docstring description multiline parsing #476

Are you sure you want to change the base?

Docstring description multiline parsing #476

Conversation

thebadcoder96 commented Dec 23, 2023

thebadcoder96 commented Jan 14, 2024 • edited Loading

dbieber commented Jan 14, 2024 via email

thebadcoder96 commented Jan 14, 2024

jamesowers-cohere Feb 8, 2024

Choose a reason for hiding this comment

thebadcoder96 Feb 9, 2024

Choose a reason for hiding this comment

thebadcoder96 Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

dbieber Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

dbieber Feb 24, 2024

Choose a reason for hiding this comment

thebadcoder96 Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

thebadcoder96 commented Jan 14, 2024 •

edited

Loading

thebadcoder96 Feb 9, 2024 •

edited

Loading

dbieber Feb 24, 2024 •

edited

Loading

thebadcoder96 Feb 26, 2024 •

edited

Loading