Skip to content
This repository was archived by the owner on Aug 17, 2023. It is now read-only.

ReadTheDocs formatted documentation #12

Merged
merged 126 commits into from
Sep 24, 2020
Merged

ReadTheDocs formatted documentation #12

merged 126 commits into from
Sep 24, 2020

Conversation

robcohen
Copy link
Contributor

Overview

ReadTheDocs formatted documentation

Comments

robcohen and others added 30 commits June 11, 2020 12:36

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
add

Unverified

No user is associated with the committer email.
add

Unverified

No user is associated with the committer email.
add

Unverified

No user is associated with the committer email.
add

Unverified

No user is associated with the committer email.
add

Unverified

No user is associated with the committer email.
add

Unverified

No user is associated with the committer email.
add

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.
fsancheziohk and others added 5 commits July 31, 2020 16:41

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Update api-references.md

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.
@robcohen robcohen requested a review from KtorZ August 10, 2020 16:09

Unverified

No user is associated with the committer email.
@KtorZ
Copy link
Contributor

KtorZ commented Aug 13, 2020

I am not 100% convinced about this PR. I understand that the rationale is that we want to have a uniform approach to documentation across the various projects. I think this a good idea, although I am not a fond of sphinx, if that makes it easier to integrate everything in one repository, then so be it.

There are a few things that need adjustment on this PR though:

  1. Commits need to be squashed into one meaningful commit that explain the rationale behind this change.

  2. Anyone needs to be able to compile the documentation from the source. So we should add proper instruction in a INSTALL.md file or something, and include a requirements.txt such as:

    requirements.txt

    recommonmark==0.6.0
    Sphinx==1.8.5
    sphinx-markdown-tables==0.0.14
    sphinx-rtd-theme==0.5.0
    sphinxcontrib-websupport==1.1.2
    sphinxemoji==0.1.6
    

    Such that one can simply run pip3 install -r requirements.txt and then sphinx-build doc dist to get the documentation built.

  3. The PR breaks quite a lot of the existing formatting and tables. We've also lost support for TeX which was used to display mathematical formulas and expressions. This needs to be fixed.

Screenshot from 2020-08-13 09-29-51
Screenshot from 2020-08-13 09-31-05
Screenshot from 2020-08-13 09-31-23

  1. Some formatting used with the Hugo template now render poorly with Sphinx. So we need to also adjust this to make things less ugly.

  2. The old "user-guide" source file of the Hugo site are still there. They need to be cleaned up as well if we carry on with this PR.

All-in-all, I am not necessarily expecting you @robcohen to do these changes, but these things need to be addressed before we merge the PR (and these points were only from a quick look at the generated documentation, we need also a deeper review to make sure we are satisfied with the quality of the documentation there.

@rvl
Copy link
Contributor

rvl commented Aug 13, 2020

Perhaps look at using MyST to address some of the markup formatting issues.

@robcohen
Copy link
Contributor Author

robcohen commented Aug 13, 2020

  1. PR can be squashed on merge, right? Do all PRs need to be merged into one commit prior to merging, seems easier to do it after. Maybe this is a bors requirement I don't know.
  2. There is a requirements.txt, it's in doc/.sphinx/requirements.txt (I tried to hide it). Agreed that the docs should be able to be generated by the user, though not sure how many people will actually do that. Sphinx usually makes this possible with a Makefile, but we can just document the sphinx command to compile the docs. Instructions now exist at the bottom of the README.rst
  3. Fair point on the TeX rendering, the tables have been fixed. MathJax can be used - see example here -> https://docs.cardano.org/projects/adrestia/en/latest/key-concepts/hierarchical-deterministic-wallets.html

You need an
```eval_rst
.. math::

<TWOSPACES>(a + b)^2

```

to have a markdown file render the mathjax correctly. I think it's best if someone from adrestia reformat the equations.
Docs with heavy math should probably just be in RST format, as you can do inline math :math: example more easily without a eval_rst directive

4. This I would leave up to the devs, but can wait until the above issues are addressed.

5. Old user guide should be deprecated in my opinion, it is beautiful and well-done, but we need one source of truth and maintaining two isn't going to happen.

6. Last issue I want to raise is that I think maybe we should consider not having an adrestia section at all. Users don't know what adrestia is. Instead, we could move the SDK explanation into the https://docs.cardano.org/en/latest/getting-started/3rd-party-integration/index.html 3rd party integration section (or we can make an API/SDK page or something else). There's no code in the adrestia repo, so I think it ultimately just adds confusion. It would be easier to have an SDK/API Architecture explanation as one of the FIRST things that users see on the docs site, and I think we can reuse most of the content that currently exists. We also need a better explanation of Cardano Architecture on docs.cardano.org anyway, so this would help.

Rob Cohen added 13 commits August 13, 2020 09:05

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.
fix

Unverified

No user is associated with the committer email.

Unverified

No user is associated with the committer email.
@robcohen
Copy link
Contributor Author

@KtorZ any thoughts on the above?

Unverified

No user is associated with the committer email.
@KtorZ
Copy link
Contributor

KtorZ commented Aug 21, 2020

@robcohen I haven't gone through your latest updates. We have discussed the need to review and move forward with this PR last Wednesday, because we have a bunch of new content we'd like to write and if we should merge this before we add more stuff.

Yet, as you imagine, the focus is not really on this at the moment.

@robcohen
Copy link
Contributor Author

Understood. I would suggest merging this PR though, because right now the official documentation on docs is running off my personal repository, which doesn't look great. We can always improve later.

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Thanks @robcohen .
Let's squash-merge this and start work from there.

@rhyslbw rhyslbw merged commit c093b86 into input-output-hk:master Sep 24, 2020
@rvl rvl mentioned this pull request Sep 25, 2020
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants