Issue #184 and #239 Added types and defaults to function help text.

[MichaelCG8]

[MichaelCG8]

I've just ran pylint and it looks like I got some of the indentation wrong, so I'll fix that tomorrow. Also running pylint with a py2 interpreter doesn't like some of the py3 only tests, so I'll add pylint disable comments to those.

[MichaelCG8]

I'd handled the tests using type hints by putting them in a string and conditionally calling exec on them if the version was >3.5. I've now changed this so that they are in a conditionally imported module, following the approach taken with fire/test_components_py3.py Linting issues are addressed, so this is now ready for your feedback. Thanks! Michael

[dbieber]

Looks good!

We'll make some small changes as part of merging, but no need to take additional action on your part. 👍

One point for discussion: You truncate types and defaults with ... <clipped> I think it's cleaner without the text <clipped>. Wdyt? I also think we should be visually consistent between how we truncate long string descriptions and summaries (see custom_descriptions.GetStringTypeSummary).

[MichaelCG8]

Good suggestion. I started with <clipped> then changed it to ... <clipped>, I didn't consider just the ellipsis alone, but I agree that that is better.

I see that custom_descriptions.GetStringTypeSummary uses formatting.EllipsisTruncate, I've applied that to the code and it tidies things up a lot.

[rragundez]

Is there any update on when is this going to be merged?

[dbieber]

We ran into an error when merging initially. The long_type test fails internally: AssertionError: 'POSITIONAL ARGUMENTS\n LONG_OBJ\n Type: typing.Tuple[typing.Tuple[typing.Tuple[typing.Tuple[typing.Tupl...' not found in 'NAME\n long_type\n\nSYNOPSIS\n long_type LONG_OBJ\n\nPOSITIONAL ARGUMENTS\n LONG_OBJ\n Type: Tuple\n\nNOTES\n You can also use flags syntax for POSITIONAL ARGUMENTS'

typing.Tuple.__qualname__ is defined as "Tuple" internally, but is an AttributeError externally. Going to disable the test and merge so we can make progress.

Thanks for your patience.

[MichaelCG8]

Hi David, thanks for looking at this! Interesting about the failing test, I tested on py3.7.7 and py2.7.18 without seeing the failure, was this seen on another python version? Or are you running these tests with some additional stuff merged in since I forked?

[dbieber]

It's Python 3.6.7 that it's failing on, but I think we may be using a nonstandard typing library. It passes on Travis, but fails on the Google testing infrastructure. When I run import typing; typing.Tuple.__qualname__ in Python 3.7.7, I get an AttributeError (which would make the test pass.) When I run import typing; typing.Tuple.__qualname__ in the Google-internal Python 3.6.7, it gives "Tuple" (which causes the test failure.)

Or are you running these tests with some additional stuff merged in since I forked?

I made a few small changes but nothing substantial. Hopefully early in the week I'll merge the PR with those small changes.

[dbieber]

Merged with 79d85df2706b2dab5dd5075f2a76953743ee9bf3

[dbieber]

As noted in the previous comment https://github.com/google/python-fire/pull/251#issuecomment-658325840 this was already merged in Jul 2020. Closing. The GitHub status of the PR will unfortunately not reflect that this was an accepted PR, (though the commit is attributed correct) -- sorry about that.