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

Multi-line parameter descriptions in Google-style docstrings #261

Open
lukedeo opened this issue May 21, 2020 · 9 comments
Open

Multi-line parameter descriptions in Google-style docstrings #261

lukedeo opened this issue May 21, 2020 · 9 comments
Labels

Comments

@lukedeo
Copy link

@lukedeo lukedeo commented May 21, 2020

Hey there! Quick question regarding the behavior of multi-line parameter descriptions in Google-style docstrings.

Consider the following code:

# main.py
def foo(bar):
    """
    This does a foo on a bar.

    Args:
        bar (str): Though a simple parameter, this parameter will need a lot of
            verbiage to describe, requiring a second line.
    """
    print(f"Hi, {bar}!")

if __name__ == "__main__":
    import fire

    fire.Fire(foo)

When running main.py --help, the resulting man is:

NAME
    main.py - This does a foo on a bar.

SYNOPSIS
    main.py BAR

DESCRIPTION
    This does a foo on a bar.

POSITIONAL ARGUMENTS
    BAR
        Though a simple parameter, this parameter will need a lot of

NOTES
    You can also use flags syntax for POSITIONAL ARGUMENTS

Notice that we are losing the following line here.

I'd expect the newline-containing description to be un-newlined (definitely not a word)

NAME
    main.py - This does a foo on a bar.

SYNOPSIS
    main.py BAR

DESCRIPTION
    This does a foo on a bar.

POSITIONAL ARGUMENTS
    BAR
        Though a simple parameter, this parameter will need a lot of verbiage to describe, requiring a second line.

NOTES
    You can also use flags syntax for POSITIONAL ARGUMENTS

Or maybe included with the newline, but in some way retaining the following line. Any plans to support this, or (likely) am I missing something?

@lukedeo lukedeo changed the title Google-style docstrings with multiple lines. Multi-line parameter descriptions in Google-style docstrings May 21, 2020
@dbieber
Copy link
Member

@dbieber dbieber commented May 21, 2020

I actually thought we supported this already - I'll mark it as a bug. Yes, we would like for this to work.

@lukedeo
Copy link
Author

@lukedeo lukedeo commented May 21, 2020

Thanks @dbieber!

@MichaelCG8
Copy link
Contributor

@MichaelCG8 MichaelCG8 commented May 22, 2020

I've seen the same using Numpy style docstrings, I'll find an example and add it later. I'll look into this tonight and see if it's something I can help with.

@MichaelCG8
Copy link
Contributor

@MichaelCG8 MichaelCG8 commented May 22, 2020

I've experimented and found the following.

def foo(bar):
"""
This does a foo on a bar.

Args:
    bar (str): Though a simple parameter, this parameter will need a lot of
        verbiage to describe, requiring a second line.
"""
print(f"Hi, {bar}!")

def foo2(bar):
  """
  This does a foo on a bar.

  Parameters
  ----------
  bar : str
      Though a simple parameter, this parameter will need a lot of
      verbiage to describe, requiring a second line.
  """

def foo3(bar):
  """
  This does a foo on a bar.

  Parameters
  ----------
  bar
    With Numpy style strings, this fails. It is strange, since it doesn't fail for foo2.
    Missing.
  """
  print(f"Hi, {bar}!")

def foo4(bar):
  """
  This does a foo on a bar.

  Parameters
  ----------
  bar
    With Numpy style strings, this fails. It is strange, since it doesn't fail for foo2.
    Missing line here.
  """
  print(f"Hi, {bar}!")

if __name__ == "__main__":
  import fire

  fire.Fire()

I access the help text using python -m bug_replication foo --help

foo is as @lukedeo described.
foo2 is a Numpy style equivalent, it renders fine.
foo3 The second line does not appear.
foo4 The second line does appear.

@MichaelCG8
Copy link
Contributor

@MichaelCG8 MichaelCG8 commented May 22, 2020

Okay, there appears to be two issues.

The issue with foo3 is that in Numpy docstrings if a line in an arg description is interpreted as the next argument if it does not contain a space or a colon. See fire.docstrings.is_arg_name(). I've updated this to include checking for a period. It would probably be more elegant to do something based on the indentation, but this is simple and fixes the issue.

The issue with foo is different. In Google docstrings if the argument name is not accompanied by a type then fire.docstrings._consume_google_args_line() records the current arg with the line state.current_arg = arg. This is missing from the case where an argument name is accompanied by a type. If you remove the type frombar in foo's docstring, then the missing part of the line is displayed correctly.

I've put the fix in a fork here https://github.com/MichaelCG8/python-fire/tree/issue-261-bugfix-multi-line-docstring-parameter-descriptions so feel free to have a look at that.

Next I'll add some tests for these cases, then I'll open a pull request.

@MichaelCG8
Copy link
Contributor

@MichaelCG8 MichaelCG8 commented May 23, 2020

Added the fix in pull request #262.
For the Numpy issue I opted for a regex comparison for a valid argument name, since it fixes more issues than just my example by checking for a period.

@dbieber I was wondering if you have a feel for the release schedule at the moment?

@dbieber
Copy link
Member

@dbieber dbieber commented May 23, 2020

Thanks for the fix!

The next feature-ful release doesn't have a date or ETA yet. The two primary goals for that are to 1) enable configs similar to what is discussed in #188 (display, serialize), and 2) enable a strict type mode, allowing Fire to benefit from type hints and prevent accidental type mismatches (such as passing an int to a function that expects a string).

We can plan smaller releases before then on an as-needed basis, such as to release bug fixes like this one. The time from bug-fix to merge to release might be a few weeks (this is a high variance, rough estimate) depending on how busy @joejoevictor and I are.

@MichaelCG8
Copy link
Contributor

@MichaelCG8 MichaelCG8 commented May 26, 2020

Hi, thanks for the info! Number 2 sounds interesting, is there an existing issue open for that?

@dbieber
Copy link
Member

@dbieber dbieber commented May 26, 2020

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

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.