[WIP]Make a common template for /api pages #18573

MSurfer20 · 2021-05-22T11:13:14Z

This PR removes the need for multiple .md files and makes a common template to generate /api pages from the OpenAPI data. This is currently a WIP.
Work plan:

Testing plan:

GIFs or screenshots:

templates/zerver/api/api-doc-template.md

zerver/openapi/zulip.yaml

zerver/openapi/markdown_extension.py

templates/zerver/help/include/rest-endpoints.md

zerver/openapi/openapi.py

zerver/openapi/zulip.yaml

MSurfer20 · 2021-05-25T00:17:44Z

Thanks for the review @timabbott ! I have addressed the comments, and am fixing the currently failing tests. The PR should be ready for another review by tomorrow.

MSurfer20 · 2021-06-23T14:38:21Z

@timabbott Managed to delete 58 files :)
EDIT: 59 now.

zerver/openapi/zulip.yaml

zerver/lib/markdown/api_return_values_table_generator.py

timabbott · 2021-06-23T23:10:49Z

zerver/openapi/markdown_extension.py

@@ -129,6 +129,8 @@ def extract_code_example(
 def render_python_code_example(
    function: str, admin_config: bool = False, **kwargs: Any
 ) -> List[str]:
+    if function not in zerver.openapi.python_examples.TEST_FUNCTIONS:
+        return []


Do we consider it a bug to have any endpoint without a Python code example? If so, we could consider raise AssertionError() here, rather than return [], so that it's not a silent failure.

Currently, the two fetch-api-keys endpoints don't have python examples -- hence the empty string.

OK, cool. I think we probably want to add them and then make this obligatory in a follow-up PR.

timabbott · 2021-06-23T23:15:14Z

I merged 4 early commits as the series ending with e892a02.

I agree that the output looks great; essentially all the changes are bug fixes :). Posted a comment on the Python commit, but I don't think it's a blocker for merging (since we can always fix that forward).

I think I agree with your thoughts on which of these can be readily deleted; we should fix the import sys thing and let the rest be.

zerver/views/documentation.py

This was a bug introduced in d0e44ea, whichw as missed due to #18573 proceeding to delete the file soon after.

MSurfer20 · 2021-06-24T00:45:47Z

@timabbott Apologies for the unnoticed bug.... Didn't realise it since all the tests had passed in the end.

timabbott · 2021-06-24T17:36:43Z

zerver/tests/test_docs.py

@@ -142,7 +142,7 @@ def test_doc_endpoints(self) -> None:
        self._test("/api/get-events", "dont_block")
        self._test("/api/delete-queue", "Delete a previously registered queue")
        self._test("/api/update-message", "propagate_mode")
-        self._test("/api/get-own-user", "takes no parameters")
+        self._test("/api/get-own-user", "does not accept any parameters.")


We don't want to have a commit where master fails tests, which the previous setup arranges. We can address this by just moving this before the previous commit, and manually changing that file to use the standard string. I'll do that while merging.

We use the "does not accept any parameters" language in the common template that we'll be migrating to shortly, so we remove this variance (And adjust its test).

The check for whether to do the special GET /events logic was incorrectly also covering DELETE /events.

Currently, the message that no parameters are accepted by the endpoint is displayed if there are no parameters in OpenAPI data, but it is possible that information is encoded in x-parameter-description (example in upload-file endpoint), and we want to display that information rather than the message. Added an if condition to check the same.

As a part of goal of moving towards a common template, the hardcoded python tabs need to be removed to ensure that endpoints which don't have python examples can be covered by the common template as well. This commit also modifies the markdown extension for python examples to render empty string in case the examples don't exist, which would allow it to be called whether the endpoint has python examples or not.

The returned values of get_path function would be expanded soon, and defining a dataclass would make the code cleaner for returning and using the fields.

The ArticlePath dataclass added can now be used for the return value of get_path function to make it more extensible and robust.

This PR adds a basic .md template that is followed by lot of /api pages. Since we have recently done the migration work to ensure that our REST API documentation pages for individual endpoints are almost all identical files following a common pattern, we can now get the payoff of deleting them all in favor of a shared template. This removes 2000 lines of somewhat finicky configuration from the codebase, and thus should save significant effort when documenting new API endpoints in the future. The markdown files for endpoints or other pages which deviate from the standard template remain, and the docs are instead generated from those files using the existing system.

timabbott · 2021-06-24T17:55:38Z

Merged after the small update above and also squashing the "add template" and "use template" commits. Huge thanks for doing this giant migration @MSurfer20!

A few notes on near-term follow-ups:

We should probably switch to consistently using endpoint_path as the local variable name for e.g. /messages/{message_id} URL paths; endpoint_name is confusing for something with /s in it. It's still a little mixed right now.
We should do an update of the "writing API documentation" docs, since there's a lot of things one no longer needs to do. We can close Update the developer documentation on our OpenAPI API docs #12571 after doing so.
It's worth noting that /api/get_messsages now works just as well as /api/get-messages. That's probably a good thing; I remember someone wanting that feature. I'll post about it in #documentation.

This was a bug introduced in d0e44ea, whichw as missed due to zulip#18573 proceeding to delete the file soon after.

This effectively reverts: - commit 0c4330f (zulip#18573) - commit 274f73d (zulip#18776) It doesn’t make sense for these two syntaxes to be entangled like this. Signed-off-by: Anders Kaseorg <anders@zulip.com>

zulipbot added the size: XL label May 22, 2021

MSurfer20 force-pushed the refactor_ai_pages branch 6 times, most recently from 851cb20 to 6ae3347 Compare May 23, 2021 11:49