Skip to content

Fix ReadTheDocs build#860

Merged
Bachibouzouk merged 4 commits intodevfrom
fix/RTD-build
Apr 14, 2021
Merged

Fix ReadTheDocs build#860
Bachibouzouk merged 4 commits intodevfrom
fix/RTD-build

Conversation

@Bachibouzouk
Copy link
Copy Markdown
Collaborator

@Bachibouzouk Bachibouzouk commented Apr 14, 2021

Fix #849

Changes proposed in this pull request:

  • Add rsvgconverter in sphinx extenstions to convert badges to pdf
  • Cut better chapters for the docs

The following steps were realized, as well (if applies):

  • Use in-line comments to explain your code
  • Write docstrings to your code (example docstring)
  • For new functionalities: Explain in readthedocs
  • Write test(s) for your new patch of code (pytests, assertion debug messages)
  • Update the CHANGELOG.md
  • Apply black (black . --exclude docs/)
  • Check if benchmark tests pass locally (EXECUTE_TESTS_ON=master pytest)

For more information on how to contribute check the CONTRIBUTING.md.

@ciaradunks
Copy link
Copy Markdown
Contributor

I've looked through each commit and basically everything looks good! A few things before I accept though:

  1. I don't know if it's the same with you or if this is intentional at the moment, but there is no content in the 'Installation' page when I build RTD locally
  2. On the index page, the Model Reference and API Reference sections look like this:
    grafik
    Maybe it would look better if either both lists were bulleted or neither were
  3. Does it need to be included that you have added 'sphinxcontrib-svg2pdfconverter' in the requirements in the changelog, or is that unecessary?

@Bachibouzouk
Copy link
Copy Markdown
Collaborator Author

I've looked through each commit and basically everything looks good! A few things before I accept though:

1. I don't know if it's the same with you or if this is intentional at the moment, but there is no content in the 'Installation' page when I build RTD locally

I don't know either, maybe refresh your local browser? The online build works well

2. On the index page, the Model Reference and API Reference sections look like this:
   ![grafik](https://user-images.githubusercontent.com/53213814/114706359-24b51800-9d29-11eb-8a30-a798f744c6d0.png)
   Maybe it would look better if either both lists were bulleted or neither were

Agreed

3. Does it need to be included that you have added 'sphinxcontrib-svg2pdfconverter' in the requirements in the changelog, or is that unecessary?

That was a mistake from me, I corrected it

If you are reviewing a PR and the person could merge it after implementing your suggestion, it is best to comment and still approve it so that approval does not need to be awaited. It kinda depends on who's writing the PR, for brand new developer you might still want to double check, or if the PR is on master for an important release then it is ok to make sure everything is perfect before approval ;)

@Bachibouzouk Bachibouzouk merged commit e7f5992 into dev Apr 14, 2021
@Bachibouzouk Bachibouzouk deleted the fix/RTD-build branch April 14, 2021 18:13
@ciaradunks
Copy link
Copy Markdown
Contributor

@Bachibouzouk yeah that's very logical! Will do that from now on

@smartie2076 smartie2076 mentioned this pull request May 31, 2021
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.

Fix ReadTheDocs compilation and allow pdf download

2 participants