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

(WIP) Pass parsed docstring object to Mako template #10

Merged
merged 7 commits into from
Oct 6, 2020

Conversation

kylebarron
Copy link
Contributor

@kylebarron kylebarron commented Aug 28, 2020

This PR aims to fix #4 by using docstring_parser to parse various types of docstrings (it claims it supports ReST, Google, and Numpydoc-style docstrings) and pass the parsed object to the Mako template.

I figured out some Mako syntax errors, but I'm still getting issues with the new parsed_docstring object not being on the object passed to Mako and haven't figure out where to add it yet.

> pdocs as_markdown -ov rio_tiler
Traceback (most recent call last):
  File "/Users/kyle/local/anaconda3/bin/pdocs", line 8, in <module>
    sys.exit(__hug__.cli())
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/hug/api.py", line 441, in __call__
    result = self.commands.get(command)()
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/hug/interface.py", line 650, in __call__
    raise exception
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/hug/interface.py", line 646, in __call__
    result = self.output(self.interface(**pass_to_function), context)
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/hug/interface.py", line 129, in __call__
    return __hug_internal_self._function(*args, **kwargs)
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/pdocs/api.py", line 83, in as_markdown
    pdocs.static.md_out(destination, roots, source=not exclude_source)
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/pdocs/static.py", line 88, in md_out
    out = pdocs.render.text(m, source=source)
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/pdocs/render.py", line 86, in text
    text, _ = re.subn("\n\n\n+", "\n\n", t.render(module=mod, show_source_code=source).strip())
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/mako/template.py", line 476, in render
    return runtime._render(self, self.callable_, args, data)
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/mako/runtime.py", line 883, in _render
    **_kwargs_for_callable(callable_, data)
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/mako/runtime.py", line 920, in _render_context
    _exec_template(inherit, lclcontext, args=args, kwargs=kwargs)
  File "/Users/kyle/local/anaconda3/lib/python3.7/site-packages/mako/runtime.py", line 947, in _exec_template
    callable_(context, *args, **kwargs)
  File "_text_mako", line 77, in render_body
  File "_text_mako", line 27, in function
  File "_text_mako", line 124, in render_function
AttributeError: 'Function' object has no attribute 'parsed_docstring'

cc @sanzoghenzo

@kylebarron kylebarron changed the title WIP pass parsed docstring object to Mako template (WIP) Pass parsed docstring object to Mako template Aug 28, 2020
@kylebarron
Copy link
Contributor Author

When I add the hasattr checks in the latest commit, I don't get errors, but it appears that the parsed_docstring track is never chosen. Any ideas?

@sanzoghenzo
Copy link
Contributor

This needs some unit testing! We can narrow it down by simply checking what happens when we init a Function class (and all the other classes, btw) with some simple functions and docstrings.

I'll try to review it ASAP

@sanzoghenzo
Copy link
Contributor

I'm sorry for my typos, this is what you get when you write code without even trying it...
Here's a really simple unittest for the parsed_docstring of a Function object:

from pdocs import doc


def simple_function(arg1, arg2):
    """
    Just a simple function.

    Args:
      arg1 (str): first argument
      arg2 (int): second argument

    Returns:
      List[str]: first argument repeated second argument types.
    """
    return [arg1] * arg2


def test_parse_docstring():
    fun_doc = doc.Function("simple_function", __name__, simple_function)
    assert fun_doc.parsed_docstring is not None
    parsed = fun_doc.parsed_docstring
    assert len(parsed.params) == 2
    first_param, second_param = parsed.params
    assert first_param.arg_name == "arg1"
    assert first_param.type_name == "str"
    assert first_param.description == "first argument"
    assert first_param.is_optional == False
    assert second_param.arg_name == "arg2"
    assert second_param.type_name == "int"
    assert second_param.description == "second argument"
    assert second_param.is_optional == False

Is should be extended for any other docstring style and documentation object (module, class, etc.), but you get the idea.

@rtbs-dev
Copy link
Contributor

So great to see this! I just found portray and was testing out migrating from sphinx+napoleon. The numpydoc-strings obviously didn't work so I looked for a way to hack it, but like magic this PR already exists! Let me know if I can assist in some way.

@kylebarron
Copy link
Contributor Author

You can pull this PR locally and try it out. I didn't get it to work yet

@rtbs-dev
Copy link
Contributor

installed to my local project's environment. portray behavior has not changed (uses the numpydocs Parameters as a header due to the following underscores).

Running pdocsseems to work, though parameters themselves are not formatted nicely. E.g this docstring:

def token_to_alias(raw_text, vocab):
    """
    Replaces known tokens with their "tag" form, i.e. the alias' in some
    known vocabulary list

    Parameters
    ----------
    raw_text: pd.Series
        contains text with known jargon, slang, etc
    vocab: pd.DataFrame
        contains alias' keyed on known slang, jargon, etc.

    Returns
    -------
    pd.Series
        new text, with all slang/jargon replaced with unified representations
    """

gives this with pdocs server libname:

image

Meanwhile the portray in_browser is totally different (even with updated pdocs), like this:

image

Copy link
Contributor

@rtbs-dev rtbs-dev left a comment

Choose a reason for hiding this comment

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

text.mako was referencing an attr that may never get created (typo in docs.py)

pdocs/doc.py Outdated Show resolved Hide resolved
pdocs/doc.py Outdated Show resolved Hide resolved
@kylebarron
Copy link
Contributor Author

Oh wow that's embarrassing haha

@rtbs-dev
Copy link
Contributor

Meh, just glad I noticed! Happens to me constantly x)

If you update the pr I can install locally and test it out.

@sanzoghenzo
Copy link
Contributor

Guys, I'm really sorry for that typos, they come from my code in the related issue...
I did a review two weeks ago with the same comment, but I'm not so good with GitHub and MRs, maybe I didn't submit it?
Anyway, I'm glad you find out the problem by yourself!

% for p in func.parsed_docstring.params:
| ${p.arg_name} | ${p.type_name} | ${p.description} | ${p.default} |
% endfor
% endif
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add the other attributes/sections of parsed_docstring here, i.e. raises and returns/yields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, after the %endif, but before the %else

| ${p.arg_name} | ${p.type_name} | ${p.description} | ${p.default} |
% endfor
% endif
% else:
Copy link
Contributor

Choose a reason for hiding this comment

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

We can add the other attributes/sections of parsed_docstring here

Copy link
Contributor Author

@kylebarron kylebarron Sep 14, 2020

Choose a reason for hiding this comment

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

Nevermind, I thought you meant after the else

This else block is if parsed_docstring doesn't exist. Then we just print the string.

If there are other attributes of parsed_docstring, those would go above inside the first % if block I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, sorry, big thumbs on a somewhat unresponsive smartphone touchscreen :)

Co-authored-by: Thurston Sexton <thurston.sexton@nist.gov>
@kylebarron
Copy link
Contributor Author

maybe I didn't submit it?

Yeah I didn't see your review until today.

If you update the pr I can install locally and test it out.

@tbsexton I merged your suggested changes

@rtbs-dev
Copy link
Contributor

rtbs-dev commented Sep 14, 2020

@kylebarron for some reason the new version is still not getting passed to the mako template in the way the file would have me expect. Here's the raw html output from the above example:

<span class="k">def</span> <span class="nf">token_to_alias</span><span class="p">(</span><span class="n">raw_text</span><span class="p">,</span> <span class="n">vocab</span><span class="p">):</span>
    <span class="sd">&quot;&quot;&quot;</span>
<span class="sd">    Replaces known tokens with their &quot;tag&quot; form, i.e. the alias&#39; in some</span>
<span class="sd">    known vocabulary list</span>

<span class="sd">    Parameters</span>
<span class="sd">    ----------</span>
<span class="sd">    raw_text: pd.Series</span>
<span class="sd">        contains text with known jargon, slang, etc</span>
<span class="sd">    vocab: pd.DataFrame</span>
<span class="sd">        contains alias&#39; keyed on known slang, jargon, etc.</span>

<span class="sd">    Returns</span>
<span class="sd">    -------</span>
<span class="sd">    pd.Series</span>
<span class="sd">        new text, with all slang/jargon replaced with unified representations</span>

@kylebarron
Copy link
Contributor Author

It would seem that

if hasattr(func, 'parsed_docstring') and func.parsed_docsting

is still evaluating to False

@sanzoghenzo
Copy link
Contributor

@tbsexton this is because this MR has't touched the html mako template yet, only the text.mako, that is used for markdown output and by portray for html output.

I wrote this test using your function and it works ok.

def token_to_alias(raw_text, vocab):
    """
    Replaces known tokens with their "tag" form.
    
    i.e. the alias' in some known vocabulary list.

    Parameters
    ----------
    raw_text: pd.Series
        contains text with known jargon, slang, etc
    vocab: pd.DataFrame
        contains alias' keyed on known slang, jargon, etc.

    Returns
    -------
    pd.Series
        new text, with all slang/jargon replaced with unified representations
    """
    pass


def test_numpydocs():
    fun_doc = doc.Function("token_to_alias", __name__, token_to_alias)
    assert fun_doc.parsed_docstring is not None
    parsed = fun_doc.parsed_docstring
    assert parsed.short_description == 'Replaces known tokens with their "tag" form.'
    assert len(parsed.params) == 2
    assert len(parsed.raises) == 0
    first_param, second_param = parsed.params
    assert first_param.arg_name == "raw_text"
    assert first_param.type_name == "pd.Series"
    assert first_param.description == "contains text with known jargon, slang, etc"
    assert first_param.is_optional == False
    assert second_param.arg_name == "vocab"
    assert second_param.type_name == "pd.DataFrame"
    assert second_param.description == "contains alias' keyed on known slang, jargon, etc."
    assert second_param.is_optional == False
    returns = parsed.returns
    assert returns.description == "new text, with all slang/jargon replaced with unified representations"
    assert returns.return_name is None
    assert returns.type_name == "pd.Series"
    assert not returns.is_generator

@rtbs-dev
Copy link
Contributor

rtbs-dev commented Sep 14, 2020

@sanzoghenzo There's still some issues with running as_markdown, which i can't change (since I don't know whether text.mako was edited on relevant lines for this pr?), but i think i've tracked down to the same typo (parsed_docsting) on text.mako for further review.

The callback for calling as_markdown from the CLI ends like this:

  File "_text_mako", line 77, in render_body
  File "_text_mako", line 23, in function
  File "_text_mako", line 125, in render_function
AttributeError: 'Function' object has no attribute 'parsed_docsting'

pdocs/templates/text.mako Outdated Show resolved Hide resolved
pdocs/templates/text.mako Outdated Show resolved Hide resolved
pdocs/templates/text.mako Outdated Show resolved Hide resolved
kylebarron and others added 3 commits September 14, 2020 13:34
Co-authored-by: Thurston Sexton <thurston.sexton@nist.gov>
Co-authored-by: Thurston Sexton <thurston.sexton@nist.gov>
Co-authored-by: Thurston Sexton <thurston.sexton@nist.gov>
@@ -16,7 +16,18 @@ def ${func.name}(
${",\n ".join(func.params())}
)${returns}
```
% if hasattr(func, 'parsed_docstring') and func.parsed_docstring:
# table with arguments info
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line or comment it with <!-- --> instead of #

@@ -30,7 +41,19 @@ ${func.docstring}
```python3
${var.name}
```
% if hasattr(var, 'parsed_docstring') and var.parsed_docstring:
# table with arguments info
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line or comment it with <!-- --> instead of #

@@ -42,7 +65,18 @@ class ${cls.name}(
)
```

% if hasattr(cls, 'parsed_docstring') and cls.parsed_docstring:
# table with arguments info
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line or comment it with <!-- --> instead of #

Co-authored-by: Andrea Ghensi <andrea.ghensi@gmail.com>
Comment on lines +56 to +63
# table with arguments info
% if var.parsed_docstring.params:
| Parameter | type | description | default |
|---|---|---|---|
% for p in var.parsed_docstring.params:
| ${p.arg_name} | ${p.type_name} | ${p.description} | ${p.default} |
% endfor
% endif
Copy link
Contributor

Choose a reason for hiding this comment

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

We're rendering a single variable here, no need for a table.
also, I don't think we have a default or arg_name here, only a description and type.

@kylebarron
Copy link
Contributor Author

Sorry I'm pretty busy and won't be able to take a look until next week at the earliest

@kylebarron
Copy link
Contributor Author

@sanzoghenzo @tbsexton if you want to move this forward quickly, feel free to open a new PR off of this one

@sanzoghenzo sanzoghenzo mentioned this pull request Sep 25, 2020
@timothycrosley timothycrosley merged commit fed5e04 into timothycrosley:master Oct 6, 2020
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.

Numpydoc support
4 participants