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

Docs: Replace broken, sprawling jsonschema2md output with 3 compact guides generated by Fadroma #276

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

egasimus
Copy link
Contributor

@egasimus egasimus commented Jul 23, 2023

I wrote a custom JSONSchema renderer over the last few days; it can be found here.

The script is native ES with zero dependencies, so it imposes no new limitations on the doc gen workflow. It can be copied it into the repo, or (once published), it can be consumed from a JS CDN on Node 18+.

The goal was to remove all things that are not relevant to practical usage of the contract API. For example:

As an API consumer, I care about literally none of this JSONSchema metadata which is on every page of the current docs:

Screenshot from 2023-07-23 02-16-14

And why does each string constant/enum variant need its own page just to be mentioned in the corner at the end of it?

Screenshot from 2023-07-23 02-16-34

Instead, let's keep the original JSON schemas available to anyone who has a preferred JSON Schema viewer, or wants to use the schemas for validation, etc.

But let's also generate a more concise Markdown overview of each contract and make that the API reference. Here's what those Markdowns look like, as of today.

Invaluable benefit of throwing out the frameworks: once again, things start at the beginning.

Screenshot from 2023-07-23 02-43-31

Compact yet detailed descriptions, incl. enum definitions and cross-linking of non-primitive types:

Screenshot from 2023-07-23 02-44-11

All messages of a kind are in one place:

Screenshot from 2023-07-23 02-33-51

More type definitions (previously affected by #273) now render correctly - even the newlines in the tables.

Screenshot from 2023-07-23 02-34-00

Same as above but some Execute messages:

Screenshot from 2023-07-23 03-03-51

As a finisher, here's how much code it takes (total, no dependencies other than Node):

Screenshot from 2023-07-23 03-01-08

Overall, I'm quite happy with how this doc rendering thingy is turning out. I believe this format of API docs makes it way easier to find info on how to interact with the contracts, and is much closer to what a newcomer would need to get started with the OKP4 platform.

Please review this PR and point out if something is missing, wrong, or needs to be clarified further.

P.S. Progress on the interactive schema-browser that I mentioned in #273 can be followed here - and it looks even better than the Hedgedoc screenshots ;-) Since its stack is slightly more complex, for now let's see how the Markdowns go though

@egasimus
Copy link
Contributor Author

Screenshot from 2023-07-24 17-34-18

whoops... fixed

@egasimus
Copy link
Contributor Author

Huh, more build fail.

Screenshot from 2023-07-24 20-47-02

Guess it's time for me to have another go at running the whole Docusaurus locally.

Btw, kudos for sticking with Yarn 1 👍 Newer versions of that package manager completely jumped the shark. I have reasons to suspect yall would love https://pnpm.io if you gave it a try though

@egasimus
Copy link
Contributor Author

Surprise surprise - Docusaurus doesn't like the generated Markdown.

Screenshot from 2023-07-24 23-07-32

Guess it'll take a little more elbow grease. Array<string> tripped up GitHub's built-in Markdown renderer, too

@egasimus
Copy link
Contributor Author

egasimus commented Jul 24, 2023

Okay, so the docusaurus is looking good!

Might be missing a height:100% somewhere...

Screenshot from 2023-07-24 23-45-01

Not that visible in dark mode tho :) I think there might be enough room on this page to do something with the code ids and code hashes...

Screenshot from 2023-07-24 23-45-03

Pretty cool, huh?

Screenshot from 2023-07-24 23-44-11

Might do the old versions next.

Screenshot from 2023-07-24 23-44-05

I think this is ready for another CI run. Let 'er rip!

@egasimus
Copy link
Contributor Author

Huh. Any idea what this is? 😅

Screenshot from 2023-07-29 16-48-35

@ccamel
Copy link
Member

ccamel commented Jul 29, 2023

@egasimus AFAIU, the job report-new-dependencies relies on organization secrets to be authorized for posting comments in the Pull Request regarding any potential changes made to the package.json file. Nevertheless, since your repository is a fork, GitHub does not transmit these secrets for security reasons, resulting in the failure of the job (with error: Error: Parameter token or opts.auth is required).

No worries. We can go through this error and still proceed with the merge.

@ccamel
Copy link
Member

ccamel commented Jul 29, 2023

@egasimus My feedback: The result is impressive, and I love the presentation of the documentation, which is not only readable but also highly useful and capable of enhancing the Developer Experience (DX)! 🤩

After the initial review with @amimart, we identified the following areas for improvement:

@egasimus
Copy link
Contributor Author

egasimus commented Aug 5, 2023

No worries. We can go through this error and still proceed with the merge.

Hoped so! :)

I wonder if we can improve the code formatting. For example, the JSON code here: http://localhost:3000/contracts/okp4-cognitarium#executemsgdeletedata.

Yeah, this one stood out to me as well. It's the same in the input schema, missing newlines and all:

Screenshot from 2023-08-05 14-15-28

Maybe cosmwasm-schema is stripping some newlines - in that case, maybe if you doubled them in the doc string, it would fix itself automatically? Alternatively, it makes sense to move such examples out of the API docs themselves and into a tutorial/guide-type document.

Some links are not functioning correctly. For instance, in the documentation of cognitarium (http://localhost:3000/contracts/okp4-cognitarium), the link to StoreLimitsInput (http://localhost:3000/contracts/okp4-cognitarium#StoreLimitsInput) is not working.

Good catch. This is because Docusausus lowercases the anchor ids. I'll make the script do likewise, and update the PR.

@egasimus
Copy link
Contributor Author

egasimus commented Aug 5, 2023

Okay, this seems to have fixed the inner links.

  • Previously, I was only touching files under contracts/. After merging the latest changes from your main branch, this doesn't seem to work anymore, so I've applied the doc generator to all the doc versions in contracts_versioned_docs 🚀 (I still can't see where to switch the docs to an older version, though.)
  • You can now regenerate the Markdown docs by running the render.sh script in the corresponding directory after installing Yarn dependencies. 🤖

@egasimus
Copy link
Contributor Author

Hey guys, any progress on merging this?

Copy link
Contributor

@bdeneux bdeneux left a comment

Choose a reason for hiding this comment

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

Hey @egasimus ! Sorry for the delay ! That seems wonderful for the rendering 😍.

On the other hand I prefer to remove all the rendering (render.sh) generation from this docs repository. I suggest you to remove it and let the new generated documentation like you have done. The generation should be done inside the contracts repository (in the makefile), because it could be useful inside this repository to have a well formatted documentation (like is done today with json2schema). Then once the contracts is released, docs repository will fetch and updated the contracts documentation with the new generated ones.

I can take care of the contracts parts if you want.

@egasimus
Copy link
Contributor Author

On the other hand I prefer to remove all the rendering (render.sh) generation from this docs repository.

Agreed! Since you already have a setup that copies the Markdown files from contracts to docs, then it definitely makes more sense for the JSON->Markdown rendering to happen in contracts.

I can take care of the contracts parts if you want.

Yes, please proceed with what is necessary to finalize this :-)

@egasimus
Copy link
Contributor Author

egasimus commented Aug 31, 2023

I've regenerated the schema docs for old versions and added a footer containing links to the doc generation tool as well as SHA256 checksum of the source schema.

See also: axone-protocol/contracts#342

@egasimus egasimus requested a review from bdeneux August 31, 2023 07:15
@egasimus
Copy link
Contributor Author

egasimus commented Sep 5, 2023

Please decide if the footer should be removed and let me know here, so we can finally fix your API docs for good :-)

@egasimus egasimus closed this Sep 5, 2023
@egasimus egasimus reopened this Sep 5, 2023
@ccamel ccamel requested a review from antho31 September 6, 2023 07:34
@antho31
Copy link
Contributor

antho31 commented Sep 7, 2023

Let's keep as it is, with the footer :)

Copy link
Contributor

@antho31 antho31 left a comment

Choose a reason for hiding this comment

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

Good job!

Copy link
Member

@amimart amimart left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for bringing this, it has great value 🙏

Copy link
Member

@ccamel ccamel left a comment

Choose a reason for hiding this comment

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

Great! Thanks so much ❤️

@ccamel ccamel removed their assignment Sep 7, 2023
Copy link
Contributor

@bdeneux bdeneux left a comment

Choose a reason for hiding this comment

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

Thanks for this 🙏 ! Nice 👍

@amimart amimart merged commit 577fc4d into axone-protocol:main Sep 7, 2023
10 of 11 checks passed
@egasimus
Copy link
Contributor Author

egasimus commented Sep 8, 2023

My pleasure! 🙇‍♂️

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.

5 participants