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

[MichaelCG8]

No description provided.

[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

[MichaelCG8]

I've copied this directive from fire/test_components_py3.py, but haven't come across it before and can't find mention of it in pylint's documentation. If there is a more specific option such as # Lint as: >python3.5 then that would be better.

[dbieber]

This is a Google-internal directive.

We'll modify our codebase syncing so its removed from the public codebase, but for now let's keep it so things work internally too.

[dbieber]

I expect I'll get to the rest of the review this Friday.

[dbieber]

This is a Google-internal directive.

We'll modify our codebase syncing so its removed from the public codebase, but for now let's keep it so things work internally too.

[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 79d85df

[dbieber]

As noted in the previous comment #251 (comment) 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.