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

Include arguments with asterik, fix issues with type hinting #22

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kbeismann
Copy link

@kbeismann kbeismann commented Jun 5, 2019

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.

@peterewills
Copy link
Contributor

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

@peterewills
Copy link
Contributor

peterewills commented Jun 15, 2019

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

@naiquevin
Copy link
Owner

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.

@kbeismann
Copy link
Author

kbeismann commented Jul 28, 2019

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
Copy link
Author

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
Copy link
Author

kbeismann commented Aug 3, 2019

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 kbeismann closed this Aug 3, 2019
@kbeismann
Copy link
Author

Accidentally closed.

@kbeismann kbeismann reopened this Aug 3, 2019
@peterewills
Copy link
Contributor

@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.

#23

@yitang
Copy link

yitang commented Sep 2, 2020

tried this, it works great. thanks @kbeismann

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

@kbeismann
Copy link
Author

@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.

@naiquevin
Copy link
Owner

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?

@kbeismann
Copy link
Author

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).

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.

4 participants