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: convert pdf files to rst and add them to docs #319

Merged
merged 14 commits into from
Apr 21, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

  • ReadMe, Docs and GitHub maintenance

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • ReadMe, Docs and GitHub maintenance:

    • Spelling has been verified
    • Code docs are working correctly

What is the current behavior?

There are two .pdf files with documentation purposes in our repo. They are indeed very well written and provide good information to the users and contributors. However, git cannot read changes on these files, therefore it is hard to keep them on track.

What is the new behavior?

Now we can track the changes on the old `technical' documentation files. Also, I'm adding the 'equations of motion' file, which sometimes even our main developers kinda forget that it exists. By promoting it to our docs page, it will get more visibility, and we get one more step on RocketPy's docs.

Does this introduce a breaking change?

  • Yes
  • No

Other information

  • There's a little chance that the images will be broken at the first time. But don't worry, we can fix the source links right after the merge.
  • I'm simply converting the files and organizing the docs indexes. It is not my intention to modify, polish, review, the contents of such files in this pr. We can think about that later.

@Gui-FernandesBR Gui-FernandesBR added the Docs Docs and examples related label Jan 2, 2023
@Gui-FernandesBR Gui-FernandesBR linked an issue Jan 2, 2023 that may be closed by this pull request
@github-actions github-actions bot requested a review from MateusStano January 2, 2023 03:55
@Gui-FernandesBR
Copy link
Member Author

If you guys want to check a preview of the html render from the .rst file, I suggest using the following code:

import docutils.core

if __name__ == "__main__":
    docutils.core.publish_file(
        source_path="C:/Users/guiga/Documents/Github-vscode/RocketPy/docs/theory/roll_equations.rst",
        destination_path="docs/theory/roll_equations.html",
        writer_name="html",
    )

The theme and colors, however, will be different than the expected to be once merged, but you can verify how the equations will be displayed, for example.

@MateusStano
Copy link
Member

Alright! this makes for a much easier timer reviewing and editing these docs files. Here are some issues:

  • I think the name theory for the folder doesn't make much sense in English. In Portuguese it does hahaha. I liked the way it was before, with technical/aerodynamics for the roll and ellipticals equations, and maybe leave the equations_of_motions in technical

  • If you open the .rst's through GitHub all the latex equations do not get displayed correctly. You can check this by opening them directly in the branch:
    https://github.com/RocketPy-Team/RocketPy/tree/maint/convert_pdfs_to_rst/docs/theory

  • In elliptical_fins.rst:

Image doesn't have a background (I fixed it in the last commit)
image

  • In equations_of_motion.rst:

If you generate the .html file, all matrixes are broken:

image

  • In index.rst. Generating the .html ends up with a few errors

@Gui-FernandesBR
Copy link
Member Author

Thanks for reviewing it, @MateusStano !! I'll comment everything in the same order, here we go:

  • Regarding the theory/tech names: I renamed the sections as I'm not actually concerned about it right now, thanks for pointing that up
  • Github's built-in file visualizer is not supposed to render .rst files in all its complexity. Docs are not supposed to be read via github, instead, via docs page. I don't see any other solution to have those equations instead of latex. Therefore, I think this is not an issue. Can we move accepting that minor flaw?
  • Thanks for fixing the image, awesome!
  • Ok, you have a good point, idk why the matrixes are not rendering on the html. It annoys me a lot, and I'm already trying to fix it. At least, I can confirm that the source of problem is not sphinx. Hopefully the .rst docs will get me to a point.
  • Finally, regarding the "In index.rst. Generating the .html ends up with a few errors" comment, I'd like to ask more details about what errors you're having, could you attach some screenshots?

@Gui-FernandesBR
Copy link
Member Author

Ok, everything done, ready for a re-review!

The sphinx.ext.mathjax extension solved the problem regarding matrices.
I found it here: https://stackoverflow.com/questions/31861792/how-to-show-matrix-in-sphinx-docs

Thks again.

To proper test the docs, one can use the following commands:

cd docs
pip install -r requirements.txt
make html

@MateusStano
Copy link
Member

MateusStano commented Jan 10, 2023

Okay! Just one more thing:

image

Seems some equations are going out of text bounds here. Everything else seems fine

@Gui-FernandesBR
Copy link
Member Author

Okay! Just one more thing:

image

Seems some equations are going out of text bounds here. Everything else seems fine

This is happening due to some configurations at the current theme version.

Updating the pydata-sphinx-theme to a newer version allowed me to wrap these equations within the text limit.

Well, ateast I don't see how to solce that with the current pydata-sphinx-theme version.

...

Adding @giovaniceotto here ,

can we consider upgrading the pydata-sphinx-theme package version from our docs page? Currently it is fixed to pydata-sphinx-theme==0.6.3 version, but there's already a 0.12 version available, we are outdated!

Probably some problems can be generated on that transition, I can help to fix any issues that appers in the docs with new version.

@Gui-FernandesBR
Copy link
Member Author

Heym it seems that pydata/pydata-sphinx-theme#1196 solved the issue that @MateusStano caught above. Therefore I updated the pydata-sphinx-theme version in the requirements for docs. Eberything should be running again.

@MateusStano please confirm that you can build the docs locally by running the following code and then we can move to merge. The conflict can be ignored, I already checked and nothing will be lost when deleting the .pdf files.

cd docs
pip install -r requirements.txt
make html

Take a look at the preview!

image

@MateusStano
Copy link
Member

All good now! I think we might need to update the EOM documentation because of what has been developed in Liquids, but that is for another time. Feel free to merge whenever

Also I solved =the conflicts by just accepting the deletion of the .pdf file

@Gui-FernandesBR
Copy link
Member Author

Only documentation files being modified, this PR goes directly into master branch,
let's go.

@Gui-FernandesBR Gui-FernandesBR merged commit 9aa050b into master Apr 21, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the maint/convert_pdfs_to_rst branch May 26, 2023 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Docs and examples related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No more docs in binary files format
2 participants