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

Merge reference doc into the main doc #167

Merged
merged 46 commits into from
Nov 12, 2018
Merged

Merge reference doc into the main doc #167

merged 46 commits into from
Nov 12, 2018

Conversation

sandcha
Copy link
Contributor

@sandcha sandcha commented Oct 22, 2018

Fixes openfisca/openfisca-core#571

Aims to merge two official documentations into one site and generate its HTML files:

To do so:

  • Imports openfisca-core reStructuredText documentation into this openfisca-doc repository.
  • Configures HTML files generation for both .rst and .md files
    • Adds source/conf.py configuration file for Sphinx v1.5 (generated with sphinx-quickstart command)
    • Updates it with .md management library (see recommonmark and AutoStructify) and save its dependencies in requirements.txt file
  • Save generation commands in Makefile
    • Adds developer command line as dev tag (check it on http://127.0.0.1:8000/)

Todo:

  • Check that no documentation is missing (check warnings like WARNING: extension 'recommonmark' has no setup() function; is it really a Sphinx extension module?)
  • Check for GitHub pages publishing (.nojekyll file already added ; see documentation for other steps)
  • Check CircleCI steps (including pdf generation)
  • Check if listed bugs below are linked to Python version

Notes:

.html generation with Sphinx comes with some compatibility bugs:

  • Sphinx set to 1.5 after getting following exception with 1.8:
Exception occurred:
  File "/(...)/virtualenvs/c571/lib/python3.7/site-packages/docutils/nodes.py", line 567, in __getitem__
    return self.attributes[key]
KeyError: 'refuri'

And this warning with 1.4:

sphinxcontrib-httpdomain 1.7.0 has requirement Sphinx>=1.5, but you'll have sphinx 1.4 which is incompatible.

Note: v1.4 is given as example in recommonmark getting started.

Exception occurred:
  File "/(...)/virtualenvs/c571/lib/python3.7/site-packages/docutils/writers/_html_base.py", line 736, in depart_document
    assert not self.context, 'len(context) = %s' % len(self.context)
AssertionError: len(context) = 2

@fpagnoux fpagnoux force-pushed the merge-doc-c571 branch 7 times, most recently from 9047a44 to 95e356b Compare November 8, 2018 23:08
@Morendil
Copy link
Contributor

Morendil commented Nov 9, 2018

Still some broken relative links, e.g. the link under "See this example of a variable using legislation parameters." in legislation-parameters.html

@fpagnoux
Copy link
Member

fpagnoux commented Nov 9, 2018

Arg, indeed, relatives links with an anchor are processed in a really weird way: ./page.md#anchor becomes /page.html#anchor.
The links are dead in production and not in tests because we serve on a subpath.

Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

There is for sure a lot to say about how to improve the doc, especially the new Python API part, which was a confidential POC so far.

I think there's already a lot of value on making that information more widely available.

As :

  • Content hasn't changed
  • Additions are in a specific section

The overall for me is a 👍

is at least as good and usable as the previous one for users

I think it is, at least police is ok and that's IMHO the most important for reading.

Some minor issues that are not blocking :

  • Links displayed as buttons
  • Sidebar style is a bit weird

We lost one feature that's "collaborative editing", but I'm not sure of the its actual real usage.

I'm ok with it if we add the "about this documentation" to the root level of the sidebar, at the end or second level for example, so it is easily discoverable.

is maintainable by the OpenFisca team

I think it is as maintainable as it was before.

Some next steps, in "about this documentation":

  • explain what is rst
  • how PYthon API doc is generated from code.

I request changes for some leftovers and a required "about this doc" rewrite, otherwise GTM.

@fpagnoux
Copy link
Member

fpagnoux commented Nov 12, 2018

Ok I finally managed to find a solution to the broken link issue that seem to work, but it's quite ugly.

We can keep this hack and hope for a fix of the underlying recommonmark bug (I'll open an issue). The only alternative I see would be to serve on doc.openfisca.org.

@sandcha @Morendil @pblayo @maukoquiroga what do you think?

It'd be great to take the decision asap.

@bonjourmauko
Copy link
Member

@fpagnoux I'm ok with the ugly hack if we can keep https://openfisca.org/doc

@fpagnoux
Copy link
Member

@fpagnoux I'm ok with the ugly hack if we can keep https://openfisca.org/doc

Ok, can you approve the PR then 😄 ?

Copy link
Member

@bonjourmauko bonjourmauko left a comment

Choose a reason for hiding this comment

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

Noice ! Trop cool.

@fpagnoux fpagnoux merged commit 602c720 into master Nov 12, 2018
@fpagnoux fpagnoux deleted the merge-doc-c571 branch November 12, 2018 16:03
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.

4 participants