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

Update how_to_docs as per recent changes #720

Merged
merged 13 commits into from
May 12, 2023

Conversation

asolavi
Copy link
Contributor

@asolavi asolavi commented May 8, 2023

Introduction

Update instructions on doc/how_to_docs.rst. This is required given recent updates on how the documentation is compiled. It arised as a follow-up discussion from PR #694.

Changes proposed in this Pull Request

  1. Remove pypsa-earth/envs/environment.docs.yaml.
  2. Update instructions on doc/how_to_docs.rst to match the new method manually creating the environment.

See #694 (comment) for further reference.

Checklist

  • I consent to the release of this PR's code under the GPLv3 license and non-code contributions under CC0-1.0 and CC-BY-4.0.
  • I tested my contribution locally and it seems to work fine.
  • Code and workflow changes are sufficiently documented.
  • Newly introduced dependencies are added to envs/environment.yaml and envs/environment.docs.yaml.
  • Changes in configuration options are added in all of config.default.yaml and config.tutorial.yaml.
  • Add a test config or line additions to test/ (note tests are changing the config.tutorial.yaml)
  • Changes in configuration options are also documented in doc/configtables/*.csv and line references are adjusted in doc/configuration.rst and doc/tutorial.rst.
  • A note for the release notes doc/release_notes.rst is amended in the format of previous release notes, including reference to the requested PR.

@asolavi
Copy link
Contributor Author

asolavi commented May 8, 2023

@ekatef shall we change the default checklist of a PR too? Point 4 states "Newly introduced dependencies are added to envs/environment.yaml and envs/environment.docs.yaml.", but envs/environment.docs.yaml does not exist anymore.

I don't know how to change these settings.

@ekatef
Copy link
Collaborator

ekatef commented May 8, 2023

Fantastic @asolavi! Very clear and nice explanations.

The checklist for PRs is a part of pull_request_template.md placed in pypsa-earth/.github folder

Copy link
Member

@pz-max pz-max left a comment

Choose a reason for hiding this comment

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

Looks good. Please only change the little comments from me and Katia and this is ready to merge 🎉 👍

@@ -58,3 +65,8 @@ Then the following commands allow you to create the documentation locally:

This will create html files in `pypsa-earth/doc/_build/html`.
VScode provides a so called Liveserver extension such that the html file can be opened locally on your computer.

.. note::
To build the documentation, Windows users might need to replace the last command by:
Copy link
Member

Choose a reason for hiding this comment

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

I think between line 69 and 70 you need a empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you, done.

@asolavi
Copy link
Contributor Author

asolavi commented May 9, 2023

Thank you @ekatef and @pz-max for your help and feedback. The two comments have been addressed, and therefore ready to merge :).

@pz-max pz-max marked this pull request as ready for review May 9, 2023 11:39
Copy link
Member

@pz-max pz-max left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @asolavi 🥇
Waiting for @ekatef review and her merge

@pz-max pz-max requested a review from ekatef May 9, 2023 11:41

To build the documentation, Windows users might need to replace the last command by:

.../pypsa-earth/doc (pypsa-earth-docs) % ./make html
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for thinking about platform specific details. It will be definitely very helpful

Just a double-check question: should it be really "./", not "." which is a usual path separator in Windows?
Could you also explain a bit on why does this fix help?

Copy link
Contributor Author

@asolavi asolavi May 10, 2023

Choose a reason for hiding this comment

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

Hi @ekatef, good point. I summarise my findings below:

After inspection, the original instruction make html now works well on the PowerShell of my Windows laptop. This was not the case in the past (that is why I looked for the ./make html workaround). Some PowerShell configuration might have changed during this time that fixed it, but I can't tell what it is.

The command .make html does not work on my windows laptop. I get this error: make_error.txt.

Since the original command make html seems to be working fine, shall we remove the Note box?

Copy link
Collaborator

@ekatef ekatef May 10, 2023

Choose a reason for hiding this comment

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

@asolavi, thanks a lot for the investigation.

My feeling is that your findings may be a good support to get starting with sphinx. It seems from this SO discussion that the issues you experienced are quite common.

As an idea, I'd suggest to keep the note changing its content to something more general. For example:

  1. Windows users can face some problems when trying to build docs locally depending on an OS version and terminal used;
  2. a workaround definitely can be found;
  3. we can suggest to look for a solution on SO (or something alike).

Although, I'm not sure if such an instruction would be really meaningful and absolutely do not insist on it. Feel free to disagree :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ekatef, the SO link you provided is really helpful and I have added your suggestions with minor adjustments.

The intention is to keep it as a list in case other similar issues arise in the future, which could be added to the list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks great, I think. Nice idea with a list to-be-continued :)

@ekatef
Copy link
Collaborator

ekatef commented May 9, 2023

Thanks a lot @asolavi! Great work :)

Could you please address a comment related to a workflow on Windows? That is amazing that you have some hints there. My general impression is that it may be quite a challenge to get the workflow run on Windows. So, any recommendations there are highly important

@asolavi
Copy link
Contributor Author

asolavi commented May 12, 2023

Thank you @ekatef, I've introduced your suggestions. What do you think?

@ekatef ekatef merged commit 22d2a45 into pypsa-meets-earth:main May 12, 2023
@ekatef
Copy link
Collaborator

ekatef commented May 12, 2023

@asolavi great work! Thanks a lot for the contribution 🙂

@asolavi
Copy link
Contributor Author

asolavi commented May 15, 2023

Thanks for your feedback and constant support, @ekatef ! :)

@asolavi asolavi deleted the doc_instructions branch May 15, 2023 14:54
@ekatef
Copy link
Collaborator

ekatef commented May 16, 2023

Thanks for your feedback and constant support, @ekatef ! :)

@asolavi My pleasure and happy to continue :)

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.

None yet

3 participants