default values should be visible in help screen

[dbieber]

As per #114 the default value for an argument should be visible in the help screen.

Some considerations:

• If the default value is a simple type and not too long in length, we can just show it.
• If it doesn't serialize well (either because there's no good way to serialize it, or because its serialization is too long to show comfortably), then we need to determine a clean alternative to showing the default. Using the type may be one option for this alternative.

[ntjess]

Would showing type information also fit under this issue? E.g. if my function signature is def main(noDefault: str, default=True)

intuitively I would expect the --help text to show noDefault is a string type. I also suspect this information wouldn't be needed for default arguments, since their type is inferred by the default. Unless its a Union[bool, SomethingElse]...

Should I submit a separate issue for this?

[dbieber]

Yeah, good idea, let's make a new issue to track that. We might end up fixing them together though.

[ntjess]

Agreed. I just created #239. Does it look acceptable?

[dbieber]

Thanks! Looks great

[MichaelCG8]

Hi, I'm exploring fire for the first time and up for having a go at this one. Could you point me towards the best place in the code to start familiarizing my self with the help strings?

EDIT: Aha, ./fire/helptext.py, the obvious suspect.

[dbieber]

Glad you found it :) If you have further questions, don't hesitate to ask.

[MichaelCG8]

Okay, so it looks like the target section is _ArgsAndFlagsSection(), which calls _CreateFlagItem(), both in helptext.py

At the moment _CreateFlagItem() takes a docstring_info argument to include the docstring description of the argument in the help text. I propose also passing in the spec variable that is present in _ArgsAndFlagsSection(), which is an instance of fire.inspectutils.FullArgSpec. That contains defaults information, and also type information. My intention is to look into type information after this feature is complete (issue #239), so whichever approach is taken being compatible with that is a plus. As you mention above, I might put them both in at the same time.

I need to dive a bit deeper into which information is included in FullArgSpec in the different python versions.

I'll create the branch issue-184-default-values-should-be-visible-in-help-screen and start working in there. In terms of code style, I presume I should be following the guide here http://google.github.io/styleguide/pyguide.html . Additionally, from looking at the code I'm seeing

• An indent of 2 spaces
• PascalCase for function names
• Redundant parentheses around tuples in return statements (this actually contradicts that style guide section 3.3)
• No use of type hints (presumably for compatibility with pre PY3.5)

Anything else I should be aware of? Running the rest suite? Shall I stick this info into ./contributing.md while I'm at it?

[MichaelCG8]

Hi, I have a solution ready and would like to create a pull request. However, when I try to push my branch I get the error message: remote: Permission to google/python-fire.git denied to MichaelCG8. fatal: unable to access 'https://github.com/google/python-fire.git/': The requested URL returned error: 403

Do I need to be an approved pusher? Or is it preferable that I fork the project and open a pull request from there?

[dbieber]

https://github.com/google/python-fire/issues/184#issuecomment-619430755: "from looking at the code I'm seeing..."

Awesome summary.

Your assessment of the coding style is spot on. The redundant parentheses is a mistake on our part though, but we can clean that up in a separate change; let's stay consistent for now.

Shall I stick this info into ./contributing.md while I'm at it?

If you're inclined to do so that would be helpful, but keep it in a separate PR so we don't block #184 on getting contributing.md right.

https://github.com/google/python-fire/issues/184#issuecomment-619570699: "Or is it preferable that I fork the project and open a pull request from there?"

Yes, that's the preferred method. Thanks for putting together the fix!

[MichaelCG8]

Great, thanks. I've put in pull request #251.

[costincaraivan]

Hi, @dbieber is there a plan to release this somewhat soon? It's a really useful feature 👍 (thanks a lot @MichaelCG8)