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

feat: Add type hint comment support #87

Merged
merged 6 commits into from Jul 7, 2019

Conversation

untitaker
Copy link
Contributor

@untitaker untitaker commented Jul 3, 2019

Fix #13

I've made it so that there's as little risk of regression as possible for existing users by shortcutting to get_type_hints or __annotations__ if that approach works. If those return empty mappings, use typed_ast with inspect.getsource to get type hint comments. An additional advantage is that typed_ast is only an optional dependency just for type hint comments.

Fix tox-dev#13

I've made it so that there's as little risk of regression as possible
for existing users by shortcutting to `get_type_hints` or
`__annotations__` if that approach works. If those return empty
mappings, use `typed_ast` with `inspect.getsource` to get type hint
comments.
Class docstring.

Parameters:
**x** -- foo
Copy link
Contributor Author

@untitaker untitaker Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this looks like any other unresolvable type. I think we could fix that in the least lines of code by setting __annotations__ on the obj, and calling get_type_hints again to resolve the names we extracted from the type hint comments... lmk what you think of this approach

@agronholm
Copy link
Collaborator

Aside from some minor code formatting nits, this looks good to me. Thanks!

@untitaker
Copy link
Contributor Author

@agronholm feel free to point those nits out

@agronholm agronholm merged commit 1599287 into tox-dev:master Jul 7, 2019
@agronholm
Copy link
Collaborator

Parameters put on too many lines, and backticks used where a double quote would have been preferred by me. Nothing worth fussing about, really.

@eric-wieser
Copy link

It would be fantastic if this could be rolled into a PyPI release

@eric-wieser
Copy link

xref sphinx-doc/sphinx#2755

@agronholm
Copy link
Collaborator

I will do that soon once I'm done with the outstanding patches lined up for that release.

agronholm added a commit that referenced this pull request Jul 23, 2019
@agronholm
Copy link
Collaborator

This is now in v1.7.0.

@wkschwartz wkschwartz mentioned this pull request Jul 24, 2019
agronholm added a commit that referenced this pull request Jul 25, 2019
agronholm added a commit that referenced this pull request Sep 8, 2019
children = list(ast.iter_child_nodes(module))

if len(children) != 1:
logger.warning(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this check? This basically disallows doing:

def magic(router, **options):
# type: (str, Any) -> Callable[[Any], Any]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this indentation intended? I ran into this warning a bunch of times but didn't investigate which items produce two ast nodes. I am also not sure what to do with two ast nodes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just select the first function definition? (type ignore elements should be definitely filtered out) - with the current implementation if the body has type ignores in it this will fail it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what about @override?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can probably come up with something sane 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said, this doesn't handle all cases, but bailing out seemed safer than selecting a random node without knowledge of why it yielded multiple.

decorators seemed to work for me, but it would make sense to get multiple nodes for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created https://github.com/agronholm/sphinx-autodoc-typehints/pull/100/files that adds non inlined type comment support 😃

@untitaker untitaker deleted the feat/type-hint-comments branch September 11, 2019 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

type hint comment support
4 participants