-
Notifications
You must be signed in to change notification settings - Fork 124
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
Add support for overloaded functions #239
Changes from 3 commits
edb60f4
16831ec
ad89d82
e0daf89
720cfc5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,15 @@ | ||
repos: | ||
- repo: https://github.com/ambv/black | ||
 rev: 18.9b0 | ||
 hooks: | ||
 - id: black | ||
language_version: python3.6 | ||
- repo: https://github.com/psf/black | ||
rev: 19.10b0 | ||
hooks: | ||
- id: black | ||
language_version: python3 | ||
- repo: https://github.com/PyCQA/pylint | ||
rev: 'pylint-2.5.3' | ||
hooks: | ||
- id: pylint | ||
- repo: https://github.com/pre-commit/pre-commit-hooks | ||
rev: v1.2.3 | ||
rev: v3.1.0 | ||
hooks: | ||
- id: trailing-whitespace | ||
- id: end-of-file-fixer | ||
- id: trailing-whitespace | ||
- id: end-of-file-fixer |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,7 +98,7 @@ def parse_assign(self, node): | |
|
||
return [data] | ||
|
||
def parse_classdef(self, node, data=None): | ||
def parse_classdef(self, node, data=None): # pylint: disable=too-many-branches | ||
type_ = "class" | ||
if astroid_utils.is_exception(node): | ||
type_ = "exception" | ||
|
@@ -127,8 +127,11 @@ def parse_classdef(self, node, data=None): | |
} | ||
|
||
self._name_stack.append(node.name) | ||
seen = set() | ||
overridden = set() | ||
overloads = {} | ||
# pylint: disable=too-many-nested-blocks | ||
ciscorn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
for base in itertools.chain(iter((node,)), node.ancestors()): | ||
seen = set() | ||
if base.qname() in ("__builtins__.object", "builtins.object"): | ||
continue | ||
for child in base.get_children(): | ||
|
@@ -138,14 +141,42 @@ def parse_classdef(self, node, data=None): | |
if not assign_value: | ||
continue | ||
name = assign_value[0] | ||
if not name or name in seen: | ||
|
||
if not name or name in overridden: | ||
continue | ||
seen.add(name) | ||
child_data = self.parse(child) | ||
if child_data: | ||
for single_data in child_data: | ||
single_data["inherited"] = base is not node | ||
data["children"].extend(child_data) | ||
|
||
for single_data in child_data: | ||
if single_data["type"] in ("method", "property"): | ||
if name in overloads: | ||
grouped = overloads[name] | ||
if single_data["doc"]: | ||
grouped["doc"] += "\n\n" + single_data["doc"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not aware of any real convention around how the docstring of an overloaded function should be displayed. The most common way I've seen is the way that pybind does it.
I don't think we'd need that first signature because we'll have already defined it in the This comment applies to the additions in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I totally agree that we shouldn't write docstrings in overload definitions in standard .py (not .pyi) code. There is no way to access overloads’ docstring at runtime. The only reason my code concatenates docstrings is to allow us to write documentation in .pyi stubs that cannot have actual implementations. FYI: Autodoc doesn't support .pyi yet and it completely drops docstrings of overload definitions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's stick with ignoring the docstrings of overloads and use only the docstring of the actual implementation. It seems like overloads aren't supposed to have docstrings, plus Sphinx doesn't render them that way. Authors then have more control over how the docstring is rendered because they'll be formatting it however they like in the single primary docstring. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Autoapi's pyi support is very useful and important for C-extension libraries. (Autodoc is also trying to support it (sphinx-doc/sphinx#4824)) If we completely ignore docstrings of overloads, we will be unable to write docs on Python stubs because stubs are not allowed to have actual implementations. I'll show an example: test_invalid.pyi from typing import overload, Union
def hello(a: int) -> int:
"""This is okay"""
@overload
def double(a: int) -> int: ...
@overload
def double(a: str) -> int:
def double(a: Union[str, int]) -> int:
"""DOCSTRING""" run
So we have to write the docstring in an overload. test_valid.pyi: from typing import overload, Union
def hello(a: int) -> int:
"""This is okay"""
@overload
def double(a: int) -> int: ...
@overload
def double(a: str) -> int:
"""DOCSTRING""" run
Do you have any ideas about this? I think employing only the last docstring is better than concatenating the all. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When using stubgen to generate a stub file for a C extension written with pybind, it sorts the actual implementation to be the last. |
||
if single_data["is_overload"]: | ||
grouped["signatures"].append( | ||
( | ||
single_data["args"], | ||
single_data["return_annotation"], | ||
) | ||
) | ||
else: | ||
grouped["args"] = single_data["args"] | ||
grouped["return_annotation"] = single_data[ | ||
"return_annotation" | ||
] | ||
continue | ||
if single_data["is_overload"] and name not in overloads: | ||
overloads[name] = single_data | ||
single_data["signatures"] = [ | ||
(single_data["args"], single_data["return_annotation"]) | ||
] | ||
|
||
single_data["inherited"] = base is not node | ||
data["children"].append(single_data) | ||
|
||
overridden.update(seen) | ||
|
||
self._name_stack.pop() | ||
|
||
return [data] | ||
|
@@ -159,6 +190,7 @@ def parse_functiondef(self, node): # pylint: disable=too-many-branches | |
|
||
type_ = "method" | ||
properties = [] | ||
|
||
if node.type == "function": | ||
type_ = "function" | ||
elif astroid_utils.is_decorated_with_property(node): | ||
|
@@ -193,6 +225,7 @@ def parse_functiondef(self, node): # pylint: disable=too-many-branches | |
"to_line_no": node.tolineno, | ||
"return_annotation": return_annotation, | ||
"properties": properties, | ||
"is_overload": astroid_utils.is_decorated_with_overload(node), | ||
} | ||
|
||
if type_ in ("method", "property"): | ||
|
@@ -249,15 +282,38 @@ def parse_module(self, node): | |
"all": astroid_utils.get_module_all(node), | ||
} | ||
|
||
overloads = {} | ||
top_name = node.name.split(".", 1)[0] | ||
for child in node.get_children(): | ||
if astroid_utils.is_local_import_from(child, top_name): | ||
child_data = self._parse_local_import_from(child) | ||
else: | ||
child_data = self.parse(child) | ||
|
||
if child_data: | ||
data["children"].extend(child_data) | ||
for single_data in child_data: | ||
if single_data["type"] == "function": | ||
name = single_data["name"] | ||
if name in overloads: | ||
grouped = overloads[name] | ||
if single_data["doc"]: | ||
grouped["doc"] += "\n\n" + single_data["doc"] | ||
if single_data["is_overload"]: | ||
grouped["signatures"].append( | ||
(single_data["args"], single_data["return_annotation"]) | ||
) | ||
else: | ||
grouped["args"] = single_data["args"] | ||
grouped["return_annotation"] = single_data[ | ||
"return_annotation" | ||
] | ||
continue | ||
if single_data["is_overload"] and name not in overloads: | ||
overloads[name] = single_data | ||
single_data["signatures"] = [ | ||
(single_data["args"], single_data["return_annotation"]) | ||
] | ||
|
||
data["children"].append(single_data) | ||
|
||
return data | ||
|
||
|
@@ -273,5 +329,4 @@ def parse(self, node): | |
data = self.parse(child) | ||
if data: | ||
break | ||
|
||
return data |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,13 @@ | ||
{% if obj.display %} | ||
.. function:: {{ obj.short_name }}({{ obj.args }}){% if obj.return_annotation is not none %} -> {{ obj.return_annotation }}{% endif %} | ||
{% for (args, return_annotation) in obj.signatures %} | ||
{% if loop.index0 == 0 %} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make more sense to store the overloads in an attribute instead of storing the real function signature with it as well? I feel like the overloads more of a special case of the function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the result that you have looks good. What I meant was that we should store overloads as an attribute called "overloads" on the objects, whereas at the moment you have an attribute called signatures that stores all signatures.
|
||
.. function:: {{ obj.short_name }}({{ args }}){% if return_annotation is not none %} -> {{ return_annotation }}{% endif %} | ||
|
||
{% else %} | ||
{{ obj.short_name }}({{ args }}){% if return_annotation is not none %} -> {{ return_annotation }}{% endif %} | ||
|
||
{% endif %} | ||
{% endfor %} | ||
{% if sphinx_version >= (2, 1) %} | ||
{% for property in obj.properties %} | ||
:{{ property }}: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the advantage of using the unicode values here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both
→
and\u2192
are OK. I just copied → from Sphinx's this line.By the way, the original autosummary doesn’t print return type annotations (maybe for saving spaces?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why that is. But you're right to match what Sphinx is doing so let's stick with what's here!