-
Notifications
You must be signed in to change notification settings - Fork 864
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
Add API docs #1379
Add API docs #1379
Conversation
@@ -62,14 +66,21 @@ class BlockParser: | |||
|
|||
A wrapper class that stitches the various `BlockProcessors` together, | |||
looping through them and creating an `ElementTree` object. | |||
|
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.
No strong feeling about this. We should just try and be consistent. Or not, it's not super important.
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.
Sorry, I'm not sure what you are referring to here.
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.
I meant, whether to keep trailing new lines in docstrings.
Currently it seems the project follows these rules:
- Leading and trailing space around summary.
- Trailing blank line after body.
I'm not sure what to think about this. I am annoyed this wasn't mentioned earlier. If it had been, I would have said no thanks. But now we are here and I'm not sure what I want to do. So let's start with this...
I am fine with or without the symbol types. But I guess I don't know what auto-summaries are. What would we loose there? |
Auto-summaries are the bullet point lists of submodules, classes, functions and attributes added automatically at the end of each docstring. It's not a big loss since these links also appear in the ToC (except for submodules). |
I'm inclined to just go with the community version. However, if we leave the settings in place for the now unsupported features will that work, or will we get an error? |
I do my best to always make insiders features compatible with the community version. Having these features enabled in the community version will not cause any error, the features will just do nothing. |
Okay. Let's leave the config as-is but move forward with the community version then. That way, if we decide to change in the future, we will already have the config as we want it (barring any future changes). The next step is to work through the failing tests and get them to be passing. Regarding the spellchecking test, it never runs because the build fails with a warning which comes from mcdocstrings (the build is run in strict mode). We will need to address that before moving on to spellchecking, which will be a chore in itself. As a general rule, we have always spelled out words rather than using abbreviations and we wrap anything in a code span which can conceivably be "code" to avoid the spellchecker tripping on it. I expect a bunch of edits to the doc strings will be needed to address reported spelling errors. In the few rare cases were there is a legitimate word that fails, it can be added to the custom dictionary. |
Just wanted to note that there was no Insiders program in place when I opened the issue in February 2022 🙂 I've only started it 5-6 months ago. Now working full-time on my own projects, and this is what allowed me to iterate quickly based on your feedback 🙂 |
Ah, I missed your last comment when posting mine. Alright, I'll get to making tests pass in CI 🙂 |
I've updated the templates in mkdocstrings-python to wrap parameter names (any object name really) in code spans, this way the spell checkers do not consider them. There were just a few words in docstrings that had to be corrected (indent, ints), and I replaced every occurrence of double backticks with single backticks ( |
There shouldn't be any HTML tags in an HTML `<title>` so there is no harm in stripping them. And we get the benefit of not having the tags rendered in the browser's title. See this [comment](Python-Markdown/markdown#1379 (review)).
There shouldn't be any HTML tags in an HTML `<title>` so there is no harm in stripping them. And we get the benefit of not having the tags rendered in the browser's title. See this [comment](Python-Markdown/markdown#1379 (review)).
Moving out of draft since we're already reviewing the PR. |
This is looking good. The only CI test failing is the check for a changelog entry. Adding an entry in docs/changelog.md will result in all tests passing. I know that we don't generally include changelog entries for changes which don't effect the behavior of the code, but this touches a lot of code (by adding type annotations) and a clear explanation should exist in the timeline of changes. However, before I merge, I want to do a read-through of the generated documentation. I may be making a few changes to the various docstrings. Unless I see a repeating pattern, I expect I'll make the changes myself as I find them. Of course, my time is limited to I don't know how long this will take. |
That is great, thanks. Take your time of course, there's no rush merging this 🙂 Definitely feel free to push directly on this branch again. |
I've been looking at the generated site locally and it bugs me that I wonder if we should just exclude |
If everything from I'm not sure how others handle this. In some of my projects, some objects are documented twice because I expose some of them in the top-level module. It bugs me as well but I don't really mind it, as I allow users to import from both locations. |
Something that we need to document for extension devs is the collections of processors and the specific name and precedence given to each so that an extension can properly insert itself in the proper location. Thus far, extension devs have had to read the source. But all of the relevant info is in the various
There are a few ways we could document this.
I'm thinking I would prefer the second option, but am open to the first if its an easy fix. |
I'm not sure exactly what you would like the table to look like, but I suppose it would be possible to build it dynamically, at build time, using markdown-exec or similar extensions/plugins: ```python exec="on"
# import all processors and sort them by priority
# then output a markdown table
print("Priority | Processor")
print("--- | ---")
for priority, processor in processors:
print(f"{priority} | {processor}")
``` |
Well, I worked it out with an extension. |
I believe this is sufficiently complete to merge. We can always tweak various things in future PRs. Thanks to @pawamoy for getting this going and following through over many months. Without all your work (and the mkdocstrings plugin to make it possible) this never would have happened. |
Very nice last commit, adding link to the source on GitHub 👍 You're welcome, thanks to you @waylan for your valuable feedback, we improved and fixed many things in mkdocstrings thanks to it 🙂 I enjoyed working on this PR with you! |
Ha, too bad the docs deployment job failed! |
Ignoring the auth key problem with the automated deploy, there is still an issue. We deploy using MkDocs' organization setup, which means deploying from a directory other than the root directory of the project (see linked docs for details). However, trying to build results in a failure: λ mkdocs build --config-file ../md/mkdocs.yml
INFO - Cleaning site directory
INFO - Building documentation to directory: C:\code\md\site
ERROR - Error reading page 'reference/markdown/index.md': Extension module
'scripts/griffe_extensions.py' could not be found
Traceback (most recent call last):
File "c:\code\md\venv\lib\site-packages\griffe\extensions\base.py", line 415, in _load_extension
ext_object = dynamic_import(import_path)
File "c:\code\md\venv\lib\site-packages\griffe\importer.py", line 61, in dynamic_import
module = import_module(module_path)
File "C:\Users\wlimberg\AppData\Local\Programs\Python\Python38\lib\importlib\__init__.py", line 127, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
File "<frozen importlib._bootstrap>", line 991, in _find_and_load
File "<frozen importlib._bootstrap>", line 973, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'scripts/griffe_extensions' It seems that extensions paths are not relative to the config file like they should be. Presumably this would be a bug in mkdocstrings. It should be altering the path to be absolute based on the location of the config file before passing it on to As a reminder, MkDocs validates its own known config options and ensures that paths are properly adjusted. However, as this is a config setting for a plugin, MkDocs doesn't know about it and therefore doesn't validate or adjust it. That needs to be handled by the plugin. Until this bug is fixed, we can't deploy the documentation. I would file a bug, but I'm not sure if this goes in mkdocsstring, mkdocstring-python or elsewhere. |
Ah, I see, thanks for the explanation. I wonder if MkDocs 1.5's new feature would solve this issue: plugins:
- mkdocstrings:
handlers:
python:
options:
extensions:
- !relative $config_dir/scripts/griffe_extensions.py |
That might provide a workaround. Although I would encourage you to fix the issue within your plugin and not rely on that feature. That feature needs to exist for Markdown extensions which don't have any knowledge of MkDocs. However, a MkDocs plugin certainly has knowledge of MkDocs and should work out-of-the-box with any supported configuration and/or environment. |
I mostly agree. However, now that this feature exists, I wonder if it is worth it to add code in mkdocstrings-python to transform the extensions path as relative to the config file directory. You know, the less code to maintain, the better. We should definitely document this though. I'll think a bit more about it 🙂 |
I would consider relying on that feature to be a bug. In fact, I would have objected to the feature being added if it wasn't for the fact that there is no workaround for Markdown extensions. I might have argued for the feature to only be available to Markdown extensions within the config file if I had known it was being considered before release. |
That represents a lot of boilerplate code in every plugin using file paths, recording MkDocs config file path in |
One annoying aspect is that projects like ReadTheDocs have to implement support for such custom tags, since they don't want to use MkDocs' loader to avoid running arbitrary Python code. I can't find any related issue on their bugtracker, will open a discussion. |
Well I strongly disagree. I spent a lot of time working on MkDocs config validation framework and any plugin can (and probably should) use that framework (as documented here) to address this and many other potential issues with configs. In other words, you can reuse the same validator that MkDocs uses for path based config options and get the correct expected behavior for free (see FilesystemObject and its various subclasses). But, anyway, this is getting off-topic. Back to the issue at hand... I tried using the
|
Thanks for trying, looks like a bug in mkdocstrings. I'll investigate and fix it ASAP. About the config validation framework: it's indeed great. However in the case of mkdocstrings and its handlers, it's not as straight-forward to use it. The thing is, each handler can declare its own configuration. Currently they declare it as simple dicts with almost no validation (not great, I know, though it allowed you to add your own Even then, the Python handler passes down options like |
The issue is now fixed with mkdocstrings-python 1.7.3 and Griffe 0.36.5. |
I got the docs deployed, but there appears to be a bug in the generator script. Nothing in the Also, all of my custom templates are ignored. You absolutely must deal with the relative path issue throughout your code. We constantly had the same kinds of issues with MkDocs until we forced all paths to be relative to the config file. Yes, it was more work up front, bit once we did it, it resolved a bunch of various weird edge cases from people using it in non-standard ways. |
I got the documentation deployed using the following: mkdocs build
cd ../Python-Markdown.github.io
ghp-import -p -n -b master ../md/site/ Notice that I had to use |
These two are easy fixes. Making the Griffe extension paths relative to the config file dir will take (much) more time. |
Following our discussion in #1220, this PR adds API docs to the documentation. The API docs are auto-generated with mkdocstrings. We use the Google-style in docstrings to document parameters, return values, exceptions raised, etc..
Important note: (and I realize it's a bit weird to talk about this only now, sorry) the proposed mkdocstrings configuration uses options that are only available to Insiders. Since you maintain Python-Markdown, I will gladly give you access for free. The Insiders version is not published on PyPI, it has to be installed from GitHub, or built and stored in a private index. We provide a solution to do exactly this, PyPI Insiders, but there is a bit of setup to use it locally (see docs), so I understand if you do not want to do that. The alternative is to rely on the community version of mkdocstrings-python, and override it in CI with the Insiders one (if CI does indeed build and publish docs). Or we can do nothing of that and just use the community version. In that case, the options that won't be available are:
Apologies for not mentioning this earlier. It can feel like I had an agenda behind this, but really I was just focused on the technical side of things (making the docs look good) and I totally forgot to speak about Insiders 🙇 Happy to hear your thoughts on this matter.
There are still some inconsistencies in docstrings (leading space in front of summary, trailing blank line at the end of the docstring). Imports are probably not ordered.
I've just based my PR on the branch as is, but I can of course rework commits to cleanly separate type changes (in function signatures) from docs changes.