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 type annotations in function signature #13

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

Conversation

AnjoMan
Copy link
Contributor

@AnjoMan AnjoMan commented Feb 7, 2020

This MR does two things

  • converts tests to take in readible HTML strings, and compares by parsing and asserting the same elements exist with the same attributes and text content
  • adds type annotations to the rendering of classes and function signatures

I've set up the classes so that users can target only type-annotated signatures -- e.g. adding the following custom css:

.autodoc-param-definition {
  display: flex;
}
.autodoc-signature__annotated .autodoc-param-definition {
  display: block;
  margin-left: 20px;
}

... result in each parameter being put on its own line, but only for signatures with annotations; other signatures could still be rendered as a single line

image

I kept the test refactoring in a separate commit, so you can run the tests against 410c792 and see that they pass before i do any code changes.

@AnjoMan AnjoMan force-pushed the include-type-annotations-in-function-signature branch 2 times, most recently from 0c20327 to fb0b727 Compare February 7, 2020 15:30
@AnjoMan
Copy link
Contributor Author

AnjoMan commented Feb 18, 2020

@tomchristie are you still working on this repo? or is there someone else who is maintaining it?

@tomchristie
Copy link
Owner

Thanks for digging into this!

There's a bit too much in the PR to make it easily reviewable. Would it be possible to strip it down to just tackling the "include type annotations" part?

I'm also interested in how the type annotations look in the docs - a screenshot before/after example here would be fantastic. We might(?) want to have them reduced into being click/hover context popovers, so that we don't swamp the docs when there are complex annotations.

@AnjoMan
Copy link
Contributor Author

AnjoMan commented Feb 19, 2020

I can split the PR up a bit to make it easier. I'll probably pull out the test refactoring part and send you that first, because without it I don't think it will be easy to tell how my changes affect the existing behaviour.

in terms of before-after, I could try to put something together to show you how things will look. I've tried to set up the annotations so that, if you didnt annotate your code then you'd see no change, but if you did annotate it then the docs would look similar to what the function definition would probably look like in the source of it were autoformatted (e.g. using black).

thanks for your comments!

@AnjoMan
Copy link
Contributor Author

AnjoMan commented Feb 19, 2020

@tomchristie FYI, i split out the test refactoring work here: #15

@AnjoMan
Copy link
Contributor Author

AnjoMan commented Feb 19, 2020

Ok, so i've made up some examples of how i think things should shake out. The main things I'm after here are:

  • short function signatures should continue to look nice and compact
  • long function signatures (including one that are long because of annotations) should continue to wrap like they do already
  • if a user wants, they can add styling to make long signatures (including ones with annotations)
    wrap nicely

This gets us pretty close to rendering signatures exactly as they are written in the authors code -- if they don't annotate their code, there wouldn't be any impact on them, but if they do annotate their code then it should just show up in the docs as they wrote it.

A few examples are described below; i've included a screenshot of the function definition along with what mkautodoc produces if you don't add any custom styling; i've also attached a screenshot of how it could look if the user decides to add styling.

Function with short signature

Here we have a short function signature with annotations. we want it to stay compact and easy
to read.

  • function definition
    image
  • with no custom css
    image
  • adding custom css
    image

Function With a Long Signature

Here we have a long signature with no annotations. This is how long functions are already rendered by mkautodoc -- the only thing i've changed is that I've added support for using custom css to improve the readibility by making the signature multi-line.

  • function definition:
    image
  • with no custom css
    image
  • adding custom css
    image

Signature With Lots of Type Annotations

Her we have a signature that is long because of the complicated type annotations. As you can see, mkautodocs renders it exactly like any other long signature definition -- the rendered text flows onto multiple lines. just like any other long annotation, we can write custom css to improve the readibility

  • function definition
    image
  • with no custom css
    image
  • adding custom css
    image

Custom CSS

Here is the custom css i'm using to get my definitions nicely styled

/* indent params in a long signature and put them on their own lines */
.autodoc-signature__long .autodoc-param-definition {
  display: block;
  margin-left: 20px;
}

/* indent the docstring below the signature */
.autodoc > div:not(:first-child) { margin-left: 10px;}



/* style the function signatures */
.autodoc-signature code { color: #404040;}
.autodoc-punctuation {color: grey;}

/* style the definitions such as class */
.autodoc-signature > em:first-child {font-weight: bold; font-style: initial}


/* style type annotations */
.autodoc-type-annotation {
  font-weight: 500;
  color: #9c6a0e;
}

/* style defaults */
.autodoc-param-default {
  color: gray;
}

@AnjoMan AnjoMan force-pushed the include-type-annotations-in-function-signature branch from fb0b727 to 2ce6424 Compare February 19, 2020 16:41
@AnjoMan AnjoMan changed the title Include type annotations in function signature WIP: Include type annotations in function signature Feb 19, 2020
@AnjoMan AnjoMan force-pushed the include-type-annotations-in-function-signature branch 2 times, most recently from c743cec to 4e3a528 Compare February 19, 2020 16:47
@tomchristie
Copy link
Owner

Okay, this looks fantastic!

@tomchristie
Copy link
Owner

I reckon this PR should also update the custom CSS in the README, right?... https://github.com/tomchristie/mkautodoc#4-optionally-add-styling-for-the-api-docs

@AnjoMan AnjoMan force-pushed the include-type-annotations-in-function-signature branch from 4e3a528 to 5b6b78d Compare February 20, 2020 14:44
@AnjoMan
Copy link
Contributor Author

AnjoMan commented Feb 20, 2020

Ok, i've updated the README.md example with the css for long signatures. if someone drops that into their project, heres what they'll see:

image

@tomchristie
Copy link
Owner

Wonderful.
This PR incorperates #15 too, right?
I reckon let's just drop the \ in the test multi-line strings, and then get these in.

(Maybe simplest just to merge this one and close the other I guess? I'd be okay with that now that have had the chance to review #15 in isolation.)

@AnjoMan
Copy link
Contributor Author

AnjoMan commented Feb 24, 2020

@tomchristie ok cool! i've pushed up a change to remove the leading slashes here 5536b1e. I'll close #15 as well

@AnjoMan AnjoMan changed the title WIP: Include type annotations in function signature Include type annotations in function signature Feb 24, 2020
@AnjoMan
Copy link
Contributor Author

AnjoMan commented Feb 28, 2020

@tomchristie just want to bump this -- i think I've prepped everything to get this done, or is there anything else you think we should get fixed up?

@tomchristie
Copy link
Owner

Righty. Installed this with httpx, to see how it's looking there...

Eg. rendering this page https://www.python-httpx.org/api/ ...

Screenshot 2020-03-02 at 10 20 04

There's a bunch of awkward edges in how those docs are rendering, so we need to think about how best to move foward.

Some things we might want to consider?...

  • It might make sense to have this as configurable, perhaps even defaulting to off at the moment?
  • We probably want ForwardRef('MyClass') to just document as MyClass.
  • The ~AnyStr style is surprising.
  • Internal import paths may not reflect external usage, eg. httpx._models.Response isn't the user-facing import style.

@AnjoMan
Copy link
Contributor Author

AnjoMan commented Mar 2, 2020 via email

@tomchristie
Copy link
Owner

I'm thinking of a flag in mkdocs.yml

I'd run with that as the first pass, yup.

@dhirschfeld
Copy link

For me, the types are a little hard to discern in the above. A monospace font for the types might make it a bit more legible?

mkautodoc/extension.py Outdated Show resolved Hide resolved
@AnjoMan AnjoMan force-pushed the include-type-annotations-in-function-signature branch from efc2e95 to 21e4797 Compare March 5, 2020 05:46
@AnjoMan
Copy link
Contributor Author

AnjoMan commented Mar 5, 2020

Ok, so, i've made some more progress on this

  • added a flag include_type_annotations so we can turn this on/off. default is off for now
  • better formatting of annotations -- i just pulled in the implementation from sphinx, since it seems fairly exhaustive. i opted to copy the source over because sphinx is a 2.7mb dependency with a whole bunch of related dependencies that i don't think we need

image

On the point about user-facing imports, I'm not entirely sure what we could do. i think it might be possible to look at the top-level package of the function and see if any types are importable from that package at the top-level, (e.g. sys.modules[Response.__module__].__package__' -> httpxand then try to doimport Response from httpx` using importlib. but this isn't exhaustive, since a submodule of a package could also be the user-facing import

@AnjoMan AnjoMan force-pushed the include-type-annotations-in-function-signature branch from 21e4797 to e9f4127 Compare March 5, 2020 06:05
This copies in the type annotation formatting
implementation written by the Sphinx team and
released under BSD.

The only changes that have been made are to
remove the custom types defined in the
original file, since they are not used
in the formatting code at all
@AnjoMan AnjoMan force-pushed the include-type-annotations-in-function-signature branch from e9f4127 to 42d4d5b Compare March 5, 2020 18:32
@AnjoMan
Copy link
Contributor Author

AnjoMan commented Mar 9, 2020

@tomchristie what do you think about pulling the user-facing-imports part of this out into a separate issue? i think its worth doing, but it seems like it would take a bunch of research and work to figure out, and what we have already serves the use-case of documenting functions that use standard types.

@tomchristie
Copy link
Owner

On the point about user-facing imports, I'm not entirely sure what we could do.

There's not going to be any general purpose way to figuring out what the import string ought to be. One answer could be to just always drop the leadning modules, and only document the class name? (eg. CookieJar and Response in the example above.)

@AnjoMan AnjoMan force-pushed the include-type-annotations-in-function-signature branch from c05fbbc to 844b0b8 Compare March 9, 2020 17:02
@AnjoMan AnjoMan mentioned this pull request Mar 15, 2020
@ofek
Copy link

ofek commented Mar 16, 2020

@tomchristie Does this look ok to merge?

@AnjoMan
Copy link
Contributor Author

AnjoMan commented Mar 28, 2020

@tomchristie just bumping this. i think i've covered all the issues, we have now:

  • a cleaner type-hint rendering function (no more ~Any)
  • package structure is cleaned out of annotations (package._secret_module.ClassName shows as ClassName)
  • a config option for whether to render type-hints at all, with default as false --- nothing should change for existing users unless they add the option

@aychang95
Copy link

Really looking forward to this PR, especially the type renderings!

We'll be using it almost immediately in adaptnlp as everything is type hinted and we rely on mkautodocs for class api documentation

@alanjds
Copy link

alanjds commented Jun 1, 2020

Would be nice to have this.

@dhirschfeld
Copy link

I'd love to start making use of this functionality. Is there any objection to merging this as-is and then iterating on master (if required)?

@patrick-kidger
Copy link

+1 for this functionality. :)

@dhirschfeld
Copy link

Hi @patrick-kidger 👋
At this point, I think this repo is ~unmaintained (no disrespect intended to the authors) and that the best option currently for autodoc like features for mkdocs is https://github.com/mkdocstrings/mkdocstrings

@patrick-kidger
Copy link

Thanks :) Yeah, that's my impression. (FWIW I've tried mkdocstrings but haven't managed to get it working.) Now I'm considering rolling my own... :D

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.

None yet

7 participants