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

Updates to the protocol documentation (and its generator) #3747

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

ethomson
Copy link
Member

@ethomson ethomson commented Jul 1, 2024

Summary

Running docusaurus build generates warnings on the generated protocol documentation, most are false positives, but three are correct (which were buried by the other false positives).

The false positives arise because we use our own anchors for linking things like request, response, and scalar types. However, docusaurus's link checker doesn't know about these generated anchors and emits false positives. This PR updates the generator to instead create React components to generate the titles and anchors, so that we can tell docusaurus about them. Its link checker will then not emit a false positive.

Then, the links to the types assume that we document all of them on this page. But the google.protobuf.* types are not documented by us. Create a TypeLink component that understands this difference and links to the protobuf docs for things like Timestamp.

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Run npm run build in the docs folder and ensure that there are no warnings.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@ethomson ethomson requested a review from a team as a code owner July 1, 2024 10:46
@coveralls
Copy link

Coverage Status

coverage: 52.006% (+0.009%) from 51.997%
when pulling d69f2fe on ethomson/proto_docs
into 5d519d3 on main.

eleftherias
eleftherias previously approved these changes Jul 3, 2024
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now, but there are some merge conflicts

Our generated proto documentation uses <h4>s but those are not visually
distinct from plain text; increase its size.
The generated protocol documentation has a few issues:

1. It creates its own anchors for linking things like request, response,
   and scalar types. However, docusaurus's link checker doesn't know
   about these generated anchors and emits false positives. Instead,
   create React components to generate the titles and anchors, so that
   we can tell docusaurus about these anchors.
2. The links to scalar types assume that we know about all of them. The
   google.protobuf.* types are not documented by us. Create a TypeLink
   component that understands this difference and links to the protobuf
   docs for things like Timestamp.
@ethomson
Copy link
Member Author

ethomson commented Jul 3, 2024

This looks good now, but there are some merge conflicts

rebased on main

@coveralls
Copy link

Coverage Status

coverage: 51.884%. remained the same
when pulling 1685bf6 on ethomson/proto_docs
into 924bd1a on main.

Co-authored-by: Eleftheria Stein-Kousathana <eleftheria@stacklok.com>
@coveralls
Copy link

Coverage Status

coverage: 51.884%. remained the same
when pulling b79fbe3 on ethomson/proto_docs
into 924bd1a on main.

@ethomson ethomson merged commit 6b75a77 into main Jul 3, 2024
19 checks passed
@ethomson ethomson deleted the ethomson/proto_docs branch July 3, 2024 21:02
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.

3 participants