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
Generate docs exampels for Python 3.10 and above #4339
Conversation
Please review. I mostly need feedback on the approach and end result, rather than detailed review of code/style/etc. |
Without reviewing the code at all, I think the idea is great and the approach is great. One note, I think I'll change how examples work a lot in V2, see dirty-equals for an example of what I want to do. That shouldn't affect this, I'll just copy your code or reimplement the same approach when I'm doing the docs for V2. |
Ok, this should be ready. Plesase review. I've cleaned up the implementation and allowed multiple future versions to be generated, e.g. Python 3.7, 3.9, 3.10, where it's applicable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this looks great 🎉, thanks so much.
A few small things, but my only significant suggestion is:
Could we change the default tab is the newest version? It seems this isn't possible directly in mkdocs, so options are:
- reverse options so the newest version comes first - I think this is kind of fine???
- use JS to change tab on page load, a bit 🤮 but might work
- ask mkdocs to add this feature - could be slow
@@ -1,10 +1,10 @@ | |||
from __future__ import annotations | |||
from typing import List # <-- List is defined in the module's global scope | |||
from typing import Any, List |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is import for context, please revert.
Also better to use int
to keep the meaning of the example as clear as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, but waiting for response in #4339 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced List
and Any
with HttpUrl
and added a comment
@@ -3,11 +3,12 @@ | |||
|
|||
|
|||
def this_is_broken(): | |||
# List is defined inside the function so is not in the module's | |||
# Annotations are defined inside the function so is not in the module's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to revert this, it was slightly clear to the layman.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it was changed is that List
is removed by pyupgrade in 3.9+, so I added Any
that shouldn't be removed in any case. Other option is to revert changes and add # dont-upgrade to the top of the file, but it might be unclear why there's no future versions in this example.
And the third way — rethink the whole example so it would fit both versions without confusion on any of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think better to import one of the pydantic types like HttpUrl
inside the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
I also called the function so prints get executed to showcase the error and absence of it.
please update. |
7b17339
to
052ac76
Compare
Don't know, but the good thing is that mkdocs will remember preffered tab throught the docs, so maybe it's ok for now? Please review. CI failed with mypy complaining since it can't find libraries used in docs. My guess is that since mypy previously was used only on pydantic, which didn't have any hard dependencies, it didn't complain much nor required install anything. I think mypy should really run in matrix with tests or in its own matrix (perhaps just for diffrenent python versions), since results may vary from installation to installation, but that's for another issue. So options for now:
Suggestions? |
You just need to add new sections to Lines 83 to 84 in 4fb872e
Please update. |
Code quality is not great and main intent here is to show the result.
a4b2923
to
9c1c2f5
Compare
Please review. |
this is really exciting 🎉, thanks so much for the contribution. |
Change Summary
Embedding examples workflow is simplified:
docs/build/exec_examples.py
will now:This script is complete, it should run "as is"
if applicableScreenshots
Tabs
Json output, single tab, incomplete script
Related issue number
Closes #4334
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)