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

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

Open
wants to merge 3 commits into
base: master
from

Conversation

@MichaelCG8
Copy link
Contributor

@MichaelCG8 MichaelCG8 commented Apr 26, 2020

No description provided.

@MichaelCG8
Copy link
Contributor Author

@MichaelCG8 MichaelCG8 commented Apr 26, 2020

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
Copy link
Contributor Author

@MichaelCG8 MichaelCG8 commented Apr 27, 2020

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

# See the License for the specific language governing permissions and
# limitations under the License.

# Lint as: python3

This comment has been minimized.

@MichaelCG8

MichaelCG8 Apr 27, 2020
Author Contributor

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.

This comment has been minimized.

@dbieber

dbieber Apr 27, 2020
Member

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.

Copy link
Member

@dbieber dbieber left a comment

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

# See the License for the specific language governing permissions and
# limitations under the License.

# Lint as: python3

This comment has been minimized.

@dbieber

dbieber Apr 27, 2020
Member

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
Copy link
Member

@dbieber dbieber commented May 1, 2020

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
Copy link
Contributor Author

@MichaelCG8 MichaelCG8 commented May 1, 2020

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
Copy link

@rragundez rragundez commented Jun 18, 2020

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

@dbieber
Copy link
Member

@dbieber dbieber commented Jul 10, 2020

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
Copy link
Contributor Author

@MichaelCG8 MichaelCG8 commented Jul 11, 2020

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
Copy link
Member

@dbieber dbieber commented Jul 11, 2020

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
Copy link
Member

@dbieber dbieber commented Jul 14, 2020

Merged with 79d85df

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.