rl-institut / multi-vector-simulator

Multi-vector Simulation Tool assessing and optimizing Local Energy Systems (LES) for the E-LAND project
GNU General Public License v2.0
21 stars 10 forks source link

Fix ReadTheDocs build #860

Closed Bachibouzouk closed 3 years ago

Bachibouzouk commented 3 years ago

Fix #849

Changes proposed in this pull request:

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

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

ciaradunks commented 3 years ago

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 commented 3 years ago

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 ;)

ciaradunks commented 3 years ago

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