sphinx-doc.el icon indicating copy to clipboard operation
sphinx-doc.el copied to clipboard

Include arguments with asterik, fix issues with type hinting

Open kbeismann opened this issue 6 years ago • 12 comments

I added a variable that toggles the inclusion of arguments with an asterik, like *args or **kwargs.

At the moment, type hinting breaks the sphinx-doc function. I modified the regex strings to make them ignore hints while still giving the expected output.

kbeismann avatar Jun 05 '19 20:06 kbeismann

Would be great to have this in master! Thanks for doing this.

peterewills avatar Jun 15 '19 18:06 peterewills

Would also be great to have the type hints included in the docstring, so that when you do spinx-doc on

def foo(a: int):
    pass

you get

def foo(a: int):
    """FIXME! briefly describe function

    :param a: 
    :type a: int
    :returns: 
    :rtype: 

    """
    pass

peterewills avatar Jun 15 '19 18:06 peterewills

Thanks for the PR. It looks good to me on first glance but I'll just run tests once and then merge it. Also, I do want to add support for type hints but I am myself don't use type hints much yet so this has been pending for a while.

naiquevin avatar Jun 25 '19 02:06 naiquevin

Thanks for the PR. It looks good to me on first glance but I'll just run tests once and then merge it. Also, I do want to add support for type hints but I am myself don't use type hints much yet so this has been pending for a while.

There are still a few issues. Most importantly, this solution doesn't recognize [ and ] as in

import typing

def foo(a: typing.Union[str, int]) -> typing.Union[str, int]:
    return a

I will add a workaround in the next days.

kbeismann avatar Jul 28 '19 14:07 kbeismann

Would also be great to have the type hints included in the docstring, so that when you do spinx-doc on

def foo(a: int):
    pass

you get

def foo(a: int):
    """FIXME! briefly describe function

    :param a: 
    :type a: int
    :returns: 
    :rtype: 

    """
    pass

I'd love to implement that, but my Lisp skills are not sufficient to solve that in a reasonable time frame. If you happen to have a good starting point in the code though, let me know.

kbeismann avatar Jul 28 '19 14:07 kbeismann

I pushed some changes so more complex type hints involving brackets and brackets in brackets are recognized properly by sphinx-doc-fun-arg-hint-regex. The first commit had issues with:

from typing import Union

def a_function(arg0: Union[str, list[list[int]], arg1: Union[int, list]) -> None:
    return None

There were two issues:

  1. Not ignoring inner closing brackets and not recognizing the last closing bracket as the end of the hint.
  2. Not recognizing multiple hints with brackets in the same row.

The second commit seems to work as expected. After M-x sphinx-doc, the result should now be:

def a_function(arg0: Union[str, list[list[int]], arg1: Union[int, list]) -> None:
    """DESCRIBE FUNCTION HERE...

    :param arg0: 
    :param arg1: 
    :returns: 

    """
    return None

Additionally, I added a variable that makes it possible to remove rtype from the initial docstring. If (setq sphinx-doc-exclude-rtype t) is added to .emacs, rtype is not inserted by default. The reasoning behind this is that for some users, type hints in the definition make types in the docstring redundant. This seems more consistent (and faster) as long as sphinx-doc.el is not able to insert hints from the definition in the docstring.

Lastly, I included a fix to ignore a non-existing parameter when there is a trailing comma at the end of a multiline definition as in:

def another_function(arg0,
                   arg1,
) -> None:
    return None

The result should now be:

def another_function(arg0,
                  arg1,
) -> None:
    """DESCRIBE FUNCTION HERE...

    :param arg0: 
    :param arg1: 
    :returns: 

    """
    return None

instead of:

def another_function(arg0,
                  arg1,
) -> None:
    """DESCRIBE FUNCTION HERE...

    :param arg0: 
    :param arg1:
    :param :
    :returns: 

    """
    return None

kbeismann avatar Aug 03 '19 13:08 kbeismann

Accidentally closed.

kbeismann avatar Aug 03 '19 13:08 kbeismann

@kbeismann I've made some changes in my fork that allow for parsing of hint types. I don't have your nice feature of having rtype be optional. Perhaps you could combine our two diffs to get one that encapsulates it all? It also looks like you might have thought about some edge cases that would break my regex. For example, I don't think the trailing comma is allowed by my regex.

It would be good to write some tests for this, but I haven't gotten around to it yet.

https://github.com/naiquevin/sphinx-doc.el/pull/23

peterewills avatar Aug 17 '19 15:08 peterewills

tried this, it works great. thanks @kbeismann

it would be great to have this in the main branch.

yitang avatar Sep 02 '20 22:09 yitang

@yitang Great if this works for you. I noticed that a few corner cases are still not covered and I should list them somewhere.

Sadly, I neglected implementing these fixes with the test suit in mind. I'd like to rectify that but I'm not yet familiar with test-driven development in Elisp.

kbeismann avatar Sep 03 '20 13:09 kbeismann

Hi @kbeismann,

Thanks for the PR and sorry for the late response. I am catching up with all the open PRs in this repo. It looks like there are multiple PRs for adding/handling type hints. I think it will be easier for me to merge PR #23 and #27 first and then cherry-pick the "include arguments with asterix" related commits from this one.

I understand there are some edge cases regarding type hints that aren't covered in either PRs. Those can be fixed later. What do you think?

naiquevin avatar Feb 01 '21 06:02 naiquevin

Hi @kbeismann,

Thanks for the PR and sorry for the late response. I am catching up with all the open PRs in this repo. It looks like there are multiple PRs for adding/handling type hints. I think it will be easier for me to merge PR #23 and #27 first and then cherry-pick the "include arguments with asterix" related commits from this one.

Hi @naiquevin,

Sounds good, I'd like to implement it test-based as proposed by @peterewills but haven't gotten around to it. I'll create another branch for that after you merged #23 and #27 into master I'd say.

I understand there are some edge cases regarding type hints that aren't covered in either PRs. Those can be fixed later. What do you think?

Yeah, sadly there are still some cases that are not covered yet. We can deal with those later (by extending the tests).

kbeismann avatar Feb 02 '21 08:02 kbeismann