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

Improve API docs #2196

Closed
sentrivana opened this issue Jun 26, 2023 · 10 comments · Fixed by #2213
Closed

Improve API docs #2196

sentrivana opened this issue Jun 26, 2023 · 10 comments · Fixed by #2213
Assignees
Labels
Component: Docs updates on getsentry/docs or docstrings Enhancement New feature or request

Comments

@sentrivana
Copy link
Contributor

sentrivana commented Jun 26, 2023

Problem Statement

Our API docs are in desperate need of some love, e.g.:

Solution Brainstorm

  • See what relevant parts are undocumented and add them.
  • Find an alternative theme where things don't overlap. Change API doc theme #2210
  • Make a general effort to improve existing docstrings.
  • Add more docstrings where it makes sense.

See also:

@untitaker
Copy link
Member

ways in which sdk api docs differ from arroyo off the top of my head:

  1. different theme
  2. get rid of sphinx-autodoc-typehints, use sphinx builtin way to render typehints (unclear if it works with type hint comments though)

about the **kwargs situation, we have a glorious hack in the sdk to share kwargs definitions between sentry_sdk.init, sentry_sdk.Client and sentry_sdk.get_options, just gotta reuse that. also works for IDEs

@sentrivana
Copy link
Contributor Author

@untitaker Trying to wrap my head around the **kwargs hacks. I got the top level capture_event to autocomplete with this:

class CaptureEventArgs(object):
    def __init__(
        self,
        event,  # type: Event
        hint=None,  # type: Optional[str]
        # (...)
        fingerprint=None,  # type: Optional[List[str]]
    ):
        # type: (...) -> None
        pass

# old capture_event
def _capture_event(*args, **kwargs):
    # type: (...) -> Optional[str]
    return Hub.current.capture_event(*args, **kwargs)

# new capture_event
if TYPE_CHECKING:
    class capture_event(CaptureEventArgs):
        pass
else:
    capture_event = (lambda: _capture_event)()

But not sure now how to get this into the api docs correctly. It doesn't show up on its own, I can explicitly tell it to document the capture_event class, but then it shows up in the docs as a class (much surprise). Do you maybe have an idea how to make this work?

Also still figuring out how to also share the CaptureEventArgs with the hub and client methods.

@untitaker
Copy link
Member

but then it shows up in the docs as a class (much surprise)

ah yeah. i forgot about that. i think i might have just lived with it back then for get_options. I think it might be possible to re-bind __init__ like so, but that's just a random idea, haven't tried it out:

class capture_event(CaptureEventArgs):
    pass

capture_event = capture_event.__init__

even if sphinx picks that up as a function to document, now the return type will be wrong :(

@sentrivana
Copy link
Contributor Author

sentrivana commented Jun 27, 2023

capture_event = capture_event.__init__ works 🙈

Dunno if we can make the return type work. You can just add # type: (...) -> Optional[str] to the fake class __init__ which tricks both vscode and sphinx into suggesting the correct return type, but don't think there's a way to get this past mypy. Will play with it a bit more.

@sentrivana
Copy link
Contributor Author

Nevermind, TIL @typing.no_type_check

@untitaker
Copy link
Member

untitaker commented Jun 27, 2023

I believe you can do this:

from typing import TypeVar, TYPE_CHECKING, Generic, Optional
from typing_extensions import reveal_type

if TYPE_CHECKING:
    BogusBaseclass = TypeVar("BogusBaseclass")
else:
    BogusBaseclass = object

class CaptureEventArgs(Generic[BogusBaseclass]):
    def __init__(a: int) -> BogusBaseclass:  # type: ignore
        pass

class _capture_event(CaptureEventArgs[Optional[str]]):
    pass

capture_event = _capture_event.__init__

reveal_type(capture_event)

@untitaker
Copy link
Member

Updated above example to be self-contained -- you also have to make __init__ no longer take self to make it work. Maybe it is not a good idea!

@untitaker
Copy link
Member

Actually I think it might be good enough to re-bind Hub.capture_event into global scope to copy its args, like this:

class Hub:
    def capture_event(self, event, hints=None) -> Optional[str]:
        ...


if TYPE_CHECKING:
    capture_event = Hub().capture_event

else:
    def capture_event(*args, **kwargs): ...

@sentrivana
Copy link
Contributor Author

Was able to get both autocompletion and Sphinx working with a variation of your last idea:

from functools import partial

if TYPE_CHECKING:
    capture_event = partial(Hub.capture_event, None)
else:
    def capture_event(*args, **kwargs):
        return Hub.current.capture_event(*args, **kwargs)

Without the partial sphinx refuses to process the function, complaining about an IndexError, my best guess is that it doesn't know what how to deal with Hub.capture_event being a bound method. But with the partial it looks good.

Thanks for the help, much appreciated!

@untitaker
Copy link
Member

hideous, i love it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Docs updates on getsentry/docs or docstrings Enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants