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
90 changes: 89 additions & 1 deletion fire/docstrings.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,12 @@ def parse(docstring):
state.returns.lines = []
state.yields.lines = []
state.raises.lines = []
state.line1 = None
thebadcoder96 marked this conversation as resolved.
Show resolved Hide resolved
state.line1_length = None
state.line2_first_word_length = None
state.line2_length = None
state.line3_first_word_length = None
state.max_line_length = 0

for index, line in enumerate(lines):
has_next = index + 1 < lines_len
Expand Down Expand Up @@ -269,7 +275,7 @@ def _join_lines(lines):
group_lines = []

if group_lines: # Process the final group.
group_text = ' '.join(group_lines)
group_text = '\n'.join(group_lines)
group_texts.append(group_text)

return '\n\n'.join(group_texts)
Expand Down Expand Up @@ -391,6 +397,9 @@ def _consume_google_args_line(line_info, state):
"""Consume a single line from a Google args section."""
split_line = line_info.remaining.split(':', 1)
if len(split_line) > 1:
state.line1 = line_info.line
state.line1_length = len(line_info.line)
state.max_line_length = max(state.max_line_length, state.line1_length)
first, second = split_line # first is either the "arg" or "arg (type)"
if _is_arg_name(first.strip()):
arg = _get_or_create_arg_by_name(state, first.strip())
Expand All @@ -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
thebadcoder96 marked this conversation as resolved.
Show resolved Hide resolved
line2_first_word = line_info.line.strip().split(' ')[0]
state.line2_first_word_length = len(line2_first_word)
state.line2_length = len(line_info.line)
if line_info.next.line: #check for line3
line3_arg = len(line_info.next.line.split(':', 1)) > 1
if not line3_arg: #line3 should not be an arg
line3_first_word = line_info.next.line.strip().split(' ')[0]
state.line3_first_word_length = len(line3_first_word)
else:
state.line3_first_word_length = None
else:
state.line2_first_word_length = state.line2_length = None


def _merge_if_long_arg(state):
"""Merges first two lines of the description if the arg name is too long.

Args:
state: The state of the docstring parser.
"""
actual_max_line_len = roundup(state.max_line_length)
arg_length = len(state.current_arg.name)
percent_105 = 1.05 * actual_max_line_len
long_arg_name = roundup(arg_length, 5) >= 0.4 * actual_max_line_len
if long_arg_name:
if state.line2_first_word_length:
line1_plus_first_word = state.line1_length + state.line2_first_word_length
line1_intentionally_short = roundup(line1_plus_first_word) < actual_max_line_len
line1_intentionally_long = state.line1_length >= percent_105
line2_intentionally_long = state.line2_length >= percent_105
if state.line3_first_word_length:
line2_plus_first_word = state.line2_length + state.line3_first_word_length
line2_intentionally_short = roundup(line2_plus_first_word) < actual_max_line_len
if not line1_intentionally_short and not line1_intentionally_long:
if not line2_intentionally_short and not line2_intentionally_long:
_merge_line1_line2(state.current_arg.description.lines)
elif not line1_intentionally_short and not line1_intentionally_long:
if not line2_intentionally_long:
_merge_line1_line2(state.current_arg.description.lines)


def _merge_line1_line2(lines):
"""Merges the first two lines of a list of strings.

Example:
_merge_line1_line2(["oh","no","bro"]) == ["oh no","bro"]

Args:
lines: a list of strings representing each line.
Returns:
same list but with the first two lines of the list now merged as a line.
"""
merged_line = lines[0] + " " + lines[1]
lines[0] = merged_line
lines.pop(1)
return lines


def roundup(number, multiple=10):
"""Rounds a number to the nearst multiple.

Example:
roundup(72) == 80

Args:
number: an interger type variable.
multiple: nearst multiple to round up to
Returns:
An interger value.
"""
remainder = number % multiple
if remainder == 0:
return number #already rounded
else:
return number + (multiple - remainder)


def _consume_line(line_info, state):
Expand Down Expand Up @@ -465,6 +551,8 @@ def _consume_line(line_info, state):
if state.section.title == Sections.ARGS:
if state.section.format == Formats.GOOGLE:
_consume_google_args_line(line_info, state)
if state.current_arg and line_info.previous.line == state.line1:
_merge_if_long_arg(state)
elif state.section.format == Formats.RST:
state.current_arg.description.lines.append(line_info.remaining.strip())
elif state.section.format == Formats.NUMPY:
Expand Down
127 changes: 124 additions & 3 deletions fire/docstrings_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,128 @@ def test_google_format_multiline_arg_description(self):
description='The first parameter.'),
ArgInfo(name='param2', type='str',
description='The second parameter. This has a lot of text, '
'enough to cover two lines.'),
'enough to\ncover two lines.'),
],
)
self.assertEqual(expected_docstring_info, docstring_info)

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

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.

self.assertEqual(expected_docstring_info, docstring_info)

def test_google_format_multiple_long_args(self):
docstring = """This is a Google-style docstring with multiple 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.
function_maker_handler_event_factory2: 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 multiple 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.'),
ArgInfo(name='function_maker_handler_event_factory2',
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.'),
],
)
self.assertEqual(expected_docstring_info, docstring_info)

def test_google_format_long_args_short_description(self):
docstring = """This is a Google-style docstring with long args.

Args:
param1_that_is_very_longer_than_usual: The first parameter. This has a lot of text,
enough to cover two lines.
"""
docstring_info = docstrings.parse(docstring)
expected_docstring_info = DocstringInfo(
summary='This is a Google-style docstring with long args.',
args=[
ArgInfo(name='param1_that_is_very_longer_than_usual',
description='The first parameter. This has a lot of text,'
' enough to cover two lines.'),
],
)
self.assertEqual(expected_docstring_info, docstring_info)

def test_google_format_multiple_long_args_short_description(self):
docstring = """This is a Google-style docstring with multiple long args.

Args:
param1_that_is_very_longer_than_usual: The first parameter. This has a lot of text,
enough to cover two lines.
param2_that_is_very_longer_than_usual: The second parameter. This has a lot of text,
enough to cover two lines.
"""
docstring_info = docstrings.parse(docstring)
expected_docstring_info = DocstringInfo(
summary='This is a Google-style docstring with multiple long args.',
args=[
ArgInfo(name='param1_that_is_very_longer_than_usual',
description='The first parameter. This has a lot of text,'
' enough to cover two lines.'),
ArgInfo(name='param2_that_is_very_longer_than_usual',
description='The second parameter. This has a lot of text,'
' enough to cover two lines.'),
],
)
self.assertEqual(expected_docstring_info, docstring_info)

def test_google_format_multiple_long_args_mixed_description(self):
docstring = """This is a Google-style docstring with multiple long args.

Args:
param1_that_is_very_longer_than_usual: The first parameter. This has a lot of text,
enough to cover two lines.
param2_that_is_very_longer_than_usual: The second parameter. This has a lot of text,
enough to cover more than two lines. Maybe it can even go a lot more than
second line.
param3_that_is_very_longer_than_usual: The third parameter. This has a lot of text,
enough to cover two lines.
"""
docstring_info = docstrings.parse(docstring)
expected_docstring_info = DocstringInfo(
summary='This is a Google-style docstring with multiple long args.',
args=[
ArgInfo(name='param1_that_is_very_longer_than_usual',
description='The first parameter. This has a lot of text,'
' enough to cover two lines.'),
ArgInfo(name='param2_that_is_very_longer_than_usual',
description='The second parameter. This has a lot of text,'
' enough to cover more than two lines. Maybe it can even go a lot more'
' than\nsecond line.'),
ArgInfo(name='param3_that_is_very_longer_than_usual',
description='The third parameter. This has a lot of text,'
' enough to cover two lines.'),
],
)
self.assertEqual(expected_docstring_info, docstring_info)
Expand Down Expand Up @@ -229,7 +350,7 @@ def test_numpy_format_typed_args_and_returns(self):
description='The second parameter.'),
],
# TODO(dbieber): Support return type.
returns='bool True if successful, False otherwise.',
returns='bool\nTrue if successful, False otherwise.',
)
self.assertEqual(expected_docstring_info, docstring_info)

Expand Down Expand Up @@ -257,7 +378,7 @@ def test_numpy_format_multiline_arg_description(self):
description='The first parameter.'),
ArgInfo(name='param2', type='str',
description='The second parameter. This has a lot of text, '
'enough to cover two lines.'),
'enough to cover two\nlines.'),
],
)
thebadcoder96 marked this conversation as resolved.
Show resolved Hide resolved
self.assertEqual(expected_docstring_info, docstring_info)
Expand Down