-
-
Notifications
You must be signed in to change notification settings - Fork 143
WiP: Add support for PEP 484 type hints #7
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
Conversation
This is a readability improvement, especially one type hints get added.
Ah, I used language features only available with 3.6 and later. Is that an issue? |
At first look, I find this far too abstracted for what is gained or what pdoc is meant to accomplish. While this sure looks correct, I'd honestly much prefer simpler: def annotated_params(self) -> List[Tuple[str, str, str]] with, by wrapping: def foo(_a, b: str, *args, c: int = None, _d=2, **kwargs): pass returning: [('_a', None, None),
('b', 'str', None),
('*args', None, None),
('c', 'int', 'None'),
('**kwargs', None, None)] Maybe as simple namedtuples. Then you can extend Also, Python 3.5, so no variable type annotations, dataclasses, or f-strings, unfortunately. |
@kernc FWIW, there's a |
The |
How is this coming along? I really would like to see this feature. Do you need help or anything @nbraud ? |
pdoc/__init__.py
Outdated
@@ -636,11 +638,16 @@ def _var_docstrings(doc_obj: Union['Module', 'Class'], *, | |||
return vs | |||
|
|||
|
|||
def _is_public(ident_name): | |||
def _is_public(ident): | |||
""" | |||
Returns `True` if `ident_name` matches the export criteria for an |
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.
Should be ident
@b52 I had some medical stuff happen and was entirely-useless this week. For what it's worth, I started rewriting this PR to use
|
@nbraud Hate to sound insensitive to your personal issues, but love for you to continue this. 🌞 |
@kernc It's fine, I appreciate it's been a while. I finally had the time & energy to look into it again, though it's not yet complete as I broke some tests in the process. The good news is that |
It's impossible for a positional parameter with no default value to follow one which has a default value: >>> def f(x, y=1, z): pass SyntaxError: non-default argument follows default argument
@kernc OK, I think I got minimum, viable PEP484 support going, but a seemingly-unrelated test fails:
Interestingly, the same test doesn't fail in |
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.
Thanks!
Besides the breaking test, there are some Python styling issues flake8 warns about.
@@ -3,6 +3,7 @@ build | |||
*.eggs/* | |||
*.egg-info | |||
*.py[cod] | |||
/.eggs/ |
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.
Is this line necessary with *.eggs/*
already specified three lines above?
|
||
func = pdoc.Function('f', pdoc.Module(pdoc), | ||
lambda _ok, a, _a, *args, _b=None, c=None, _d=None: None) | ||
self.assertEqual(func.params(), ['_ok', 'a', '_a', '*args', 'c=None']) | ||
self.assertEqual([str(p) for p in func.params()], ['_ok', 'a', '_a', '*args', 'c = None']) |
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.
PEP 8 suggests to use spaces around equals sign only when type is specified:
def munge(input: AnyStr, sep: AnyStr = None, limit=1000): ...
@@ -1366,62 +1414,30 @@ def _is_async(self): | |||
return False | |||
|
|||
@lru_cache() | |||
def params(self) -> List[str]: | |||
def params(self) -> List[Union[str, Parameter]]: |
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'm strongly opposed to breaking established API by returning something else than strings. It makes no sense, really, as any code that were to do something more than str(param)
will have to instance check.
Maybe is better:
def params(self, *, types: bool = False) -> List[str]
with types=True
returning the strings interpolated with typing info?
pdoc API-wise, I think it's easy enough to let the user do on their own if needed:
inspect.signature(pdoc_func_obj.obj)
In that way, class Parameter(inspect.Parameter)
is not even required, in exchange for some of the former complexity of this method (should be much less with inspect
API).
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.
types=
parameter can then be a template tunable in config.mako:
pdoc/pdoc/templates/config.mako
Lines 1 to 12 in a1f0a15
<%! | |
# Template configuration. Copy over and adapt as required. | |
html_lang = 'en' | |
show_inherited_members = False | |
extract_module_toc_into_sidebar = True | |
list_class_variables_in_index = True | |
sort_identifiers = True | |
# Set the style keyword such as 'atom-one-light' or 'github-gist' | |
# Options: https://github.com/highlightjs/highlight.js/tree/master/src/styles | |
# Demo: https://highlightjs.org/static/demo/ | |
hljs_style = 'github' | |
%> |
@nbraud Do you agree with my comments? I'd love to see this brought to completion. |
Replaced with 5df06cd. Thanks anyway, @nbraud. I'd appreciate feedback whether the result is adequate. |
Fixes #6.
I started by refactoring
Function.params
to produce a list ofParameter
objects.This was necessary, as type hints aren't represented the same way in text and HTML (due to linking).
Feel free to comment on the refactor, but type hints themselves are very much a work in progress (the pushed version just adds the necessary field to
Parameter
, that's all).Function.params
.