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

Docstring description multiline parsing #476

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

Conversation

thebadcoder96
Copy link

this would potentially close #448

Please review.

fire/docstrings.py Outdated Show resolved Hide resolved
fire/docstrings.py Outdated Show resolved Hide resolved
fire/docstrings.py Outdated Show resolved Hide resolved
@thebadcoder96
Copy link
Author

thebadcoder96 commented Jan 14, 2024

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
Copy link
Member

dbieber commented Jan 14, 2024 via email

@thebadcoder96
Copy link
Author

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.

Comment on lines +173 to +191
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.'),
],
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.'),
],
)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@thebadcoder96 thebadcoder96 Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dbieber dbieber Feb 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reason for this line?

Copy link
Author

@thebadcoder96 thebadcoder96 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parsing function help with multiline
3 participants