Skip to content
This repository has been archived by the owner on Oct 14, 2023. It is now read-only.

Changed Docs to Markdown Format from reStructuredText Format #1073

Merged
merged 1 commit into from Jan 19, 2021

Conversation

DhruvSondhi
Copy link
Contributor

Hello,
This PR aims to propose the idea of migration of the documentation from .rst format to .md format for easier editing & maintenance 馃槃
Implements #977

@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 12, 2021 15:39 Inactive
@DhruvSondhi
Copy link
Contributor Author

DhruvSondhi commented Jan 14, 2021

Hello,
We have completed about 85-90% of the migration to markdown format for Read The Docs. But the following problems are arising, due to non-native support for .md file format in Sphinx Documentation Generator :

  • Upon the conversion of all the files from .rst to .md, the following files were most affected:
    1. Index.md
    2. use_guide.md
    3. search.md
  • Main Problems :
    1. index.md doesn't have the working toctree as the .rst counterpart had.
    2. index.md is also not being rendered in HTML format even though, it is being rendered in normal markdown format.
    3. If we include any reStructured Text syntax then the document doesn't render markdown syntax 馃槥
    4. All Hoverxrefextension's commands & annotations were removed by me as it doesn't render correctly { Markdown doesn't support hoverxref extenstion}
    5. user_guide.md doesn't render as the HTML page & does raise some trivial warnings :(
    6. inline syntax, from Myst & rst, raises an error which is evident in both files
    7. Again, toctree is not working & hence the referencing of the page is not possible & Index for referencing on the page remains empty
    8. nbSphinxGallery doesn't show the gallery & the thumbnails for all the examples notebooks ... Update extension nbSphinxGallery is not supported 馃槥
  • removing Directive syntax from any of the markdown generated pages, allows the page to be rendered prefectly ... Issue is with the Directive syntax parsing in Myst 馃槥
  • Will add more issues as and when I encounter them or will try to fix multiple 馃槃

UPDATE: Fixed most of the errors.
NEW PROBLEM: nbSphinxGallery

UPDATE 2: Fixed everything ... Ready to go 馃槃

@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 14, 2021 05:44 Inactive
@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

Merging #1073 (20918c2) into master (9b94129) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1073   +/-   ##
=======================================
  Coverage   89.52%   89.52%           
=======================================
  Files          75       75           
  Lines        3991     3991           
  Branches      343      343           
=======================================
  Hits         3573     3573           
  Misses        326      326           
  Partials       92       92           

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 9b94129...20918c2. Read the comment docs.

@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 16, 2021 15:05 Inactive
@astrojuanlu
Copy link
Member

The builds are failing:

ModuleNotFoundError: No module named 'myst_parser'

@DhruvSondhi could you please add the necessary modules to the dev requirements in pyproject.toml so they get picked up by https://github.com/poliastro/poliastro/blob/master/.readthedocs.yml ?

Then mark this pull request as ready for review so we can have a closer look :)

Great job fixing the remaining issues!

@DhruvSondhi DhruvSondhi marked this pull request as ready for review January 17, 2021 04:55
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 17, 2021 06:44 Inactive
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 17, 2021 06:49 Inactive
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 17, 2021 08:03 Inactive
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 17, 2021 10:25 Inactive
tox.ini Outdated Show resolved Hide resolved
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 17, 2021 10:44 Inactive
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 17, 2021 10:46 Inactive
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

This is awesome work, thanks a lot @DhruvSondhi! 馃憦馃徑 There were a lot of challenges along the way, but you almost got it working. Even the autoapi and hoverxref extensions.

However, the notebook gallery lost several thumbnails and, most importantly, cell outputs. I think we should figure this out before merging this PR.

One obstacle might be that one can't register two processors for the same extension:

Extension error:
source_suffix '.md' is already registered
make: *** [Makefile:61: html] Error 2

One silly idea that I had was to use a different extension, like .nmd, for notebooks. But I don't know if this breaks other stuff.

When we are out of silly ideas, I think we should get in touch with nbsphinx and executablebooks authors, with a Short, Self-Contained, Correct Example to reproduce the problem, and inquire about a way forward.

If combining these two systems is impossible, we should reach out to MyST-NB authors to ask whether a "gallery-like" feature is in scope.

And finally, if this is truly impossible, we could consider alternatives for the notebooks, like having only links to Binder. This has obviously pros and cons.

@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 18, 2021 05:08 Inactive
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 18, 2021 05:40 Inactive
@DhruvSondhi
Copy link
Contributor Author

DhruvSondhi commented Jan 18, 2021

After checking through all the documentation thus generated after the migration to Markdown, the following issues are found :

  • The nbSphinx gallery doesn't work (as stated above already by astrojuanlu) [Fixed]
  • The nbSphinx extension also had the functionality of executing & displaying all the outputs thus generated, but now doesn't work 馃槥 [Fixed]
  • Issue Investigate sphinx-autoapi warning聽#1077 states problems with warning related to the Autoapi extension & is linked with the Search functionality of the document being redundant as of current status 馃槥 [Not In Scope Of This PR 馃槈 ]

@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 19, 2021 12:04 Inactive
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 19, 2021 12:07 Inactive
@astrojuanlu
Copy link
Member

Closing and reopening to try to trigger pipelines.

@astrojuanlu astrojuanlu reopened this Jan 19, 2021
@astrojuanlu astrojuanlu temporarily deployed to validation-env January 19, 2021 14:06 Inactive
@astrojuanlu
Copy link
Member

@DhruvSondhi, #1077 is not linked to Search. see #1079

@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 19, 2021 14:36 Inactive
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 19, 2021 14:39 Inactive
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Awesome work! Did a first superficial review and left some comments.

To clarify: the output looks great and I'm looking forward to merging this!

docs/source/changelog.md Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved
docs/source/conf.py Show resolved Hide resolved
docs/source/getting_started.md Outdated Show resolved Hide resolved
docs/source/index.md Outdated Show resolved Hide resolved
docs/source/index.md Outdated Show resolved Hide resolved
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 19, 2021 17:06 Inactive
@DhruvSondhi DhruvSondhi temporarily deployed to validation-env January 19, 2021 17:07 Inactive
Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

This is huge and I did not review everything in depth, but after a quick look on the output, I think this is ready to be merged! If we find small formatting issues, they can be fixed afterwards.

@astrojuanlu astrojuanlu merged commit 0e0dc1e into poliastro:master Jan 19, 2021
@astrojuanlu
Copy link
Member

Congratulations @DhruvSondhi for this pull request! It was not an easy task and the final results are awesome 馃殌

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

2 participants