ossc-db / pg_hint_plan

Extension adding support for optimizer hints in PostgreSQL
Other
715 stars 103 forks source link

Overhaul the documentation #125

Closed rjuju closed 1 year ago

rjuju commented 1 year ago

As discussed at https://github.com/ossc-db/pg_hint_plan/issues/123 I worked on a new documentation system, still based on markdown format, which can then be used to generate html documentation (either locally using mkdocs or using 3rd party service like readthedocs). For simplicity I split the main README.md file into various docs/*.md file, and only kept the synopsis and a TOC with link in the README.

I split the various steps to make it easier to follow, the only interesting points IMHO are:

  1. the mkdocs.yml in the root directory
  2. the docs/index.md that copy/paste the doc structure
  3. the extra css file to avoid horizontal scrollbars on generated html

I choose to manually duplicate the TOC tree in the readme as having an automatic one isn't supported by default and I'd rather avoid relying on too many extensions.

As an example I temporarily configured https://pg-hint-plan.readthedocs.io/en/latest/ to point to this branch so you can check the generated html. The locally generated doc is strictly identical.

Let me know if you have any comments or modification you'd like to see. Also, should I backpatch this documentation change, and if yes how far? AFAICS the README.md was created in the pg14 version so it could be a good first step. If backpatch is wanted I will also make sure that RTD can handle that (it should).

michaelpq commented 1 year ago

Patch 1 is a straight-forward removal. Nothing to comment on that. Patch 2 is a copy-paste of the README, simple enough. Patch 3 is a reformatting to 80 characters per line. Thanks a lot for making that readable! I was reading through patch 4 and noticed a bunch of sentences that need to be reworked or fixed, keeping some notes around. I think that as part of this move it is fine to address only the use of "#" to correctly align the header styles and limit the use of "postgres#" in the SQL examples. Patch 5 seems also fine.

In short, +1 ;)

It really looks like the docs in English should be reworked and trimmed from all the incorrectness I found while reading them. Fine by me to do it once the split is done, and the reindentation makes that so muuuuch easier.

rjuju commented 1 year ago

I also found some typos when reformatting the docs (I pushed a few commits for that), and there are indeed a lot more. It will be much easier to work on that after merging this PR as it seems like a big project on its own, and as I also think that a 80-col width makes things easier :)

rjuju commented 1 year ago

About back-patching, is pg14 a decent enough target? Older versions don't have a README, and going from html to md isn't really motivating me.

michaelpq commented 1 year ago

About back-patching, is pg14 a decent enough target? Older versions don't have a README, and going from html to md isn't really motivating me.

My motivation would not go beyond the stable branch where I begin to see conflicts, so agreed that this should be OK. Things have not changed much in the last few years, still there may be a delta.

rjuju commented 1 year ago

I was thinking of doing the same steps manually on each branches, as they all conflict and resolving them seems more dangerous than copy/pasting stuff again.

rjuju commented 1 year ago

I've been discussing with @yamatattsu and keeping a multi-language documentation seems to be something quite likely.

I've been doing some more research and mkdocs isn't really as mature as sphinx when it comes to such feature. I will work on a new branch trying to build the same thing using sphinx and the myst-parser to keep most of the documentation in markdown.

Note that while markdown is way more readable, it's also way less powerful than restructured text. Going with sphinx might be a better option in the long run as we would be able to switch to restructured text more easily if we ever need to, even on a per-file basis.

rjuju commented 1 year ago

Sorry for the delay, I didn't have time to work on that before today. So I now have another patchset that relies on sphinx but keeps markdown compatibility thanks to the myst-parser plugin. I have to say that I've been pleasantly surprised by this combination.

I wanted to make sure that every features we're interested in would work so I also added the basis for Japanese translation, and imported a few bits of the previous translation, mostly where the English version didn't change at all or the chunks I could understand.

The updated result can be seen at https://pg-hint-plan.readthedocs.io/en/latest/

The result is much better as we can now rely on the TOC being automatically generated. It's however a bit inconsistent as the included page should start as a nested level of 1 rather than 2, but before spending time cleaning that up I want to make sure that everyone is onboard with this new version.

Note also that the version selector (bottom left) now shows a language select, with en and ja available, the Japanese version available at https://pg-hint-plan.readthedocs.io/ja/latest/.

I used a new branch for this work, available at https://github.com/rjuju/pg_hint_plan/commits/doc_v2, so you can easily compare the differences in the structure. While at it I added a lot of internal documentation on how to work with the documentation, so hopefully it should be easy to get started with either building the doc locally or eventually finishing the translation.

Let me know what you think, and if I should forcepush on this branch to update the PR or just create a new one with the sphinx approach.

michaelpq commented 1 year ago

Let me know what you think, and if I should forcepush on this branch to update the PR or just create a new one with the sphinx approach.

Force pushing on this PR is fine by me.

rjuju commented 1 year ago

Ok, done!

rjuju commented 1 year ago

FTR I pushed a naive "doc_PG15" version for en and ja, which just changed the version info in the main page to also test the multi version, and it works as expected. The version select now shows either "latest" and "doc_PG15" (which is the branch name, once merged it will just be PG15), for both en and ja versions.

As far as I'm concerned I'm happy with this tooling and I think it's the best compromise we can aim for, as we will be ready for any other fancy feature integration in the future if needed. That being said, I've worked on the pg translation for years so I'm already quite used to working with po files, which may not be the case for people interested in translating this documentation.

michaelpq commented 1 year ago

@rjuju I have been reading through your set of changes, and a good portion of them are unchanged compared to the last version, at the exception of the documentation about how to use sphinx.

+1 to apply what you have here, and work towards improving things in this direction.

rjuju commented 1 year ago

Ok!

So I took care of doing some additional fixups to remove doc build warnings, and manually did the same work for PG14 and PG15 and pushed the result on all branches!

The next thing is to know whether we want to host compiled documentation on RTD or not. If yes, I need the list of person and their RTD accounts to give permission (and possibly remove my own access if needed), and also someone then needs to add the proper webhooks on this repository. It requires more permissions than I have, and can be done easily using RTD UI.

michaelpq commented 1 year ago

Should this pull request be closed now? It seems to me that publishing the docs could be dealt with separately.

rjuju commented 1 year ago

Fine by me.