iiasa / ixmp

The ix modeling platform for integrated and cross-cutting scenario analysis
https://docs.messageix.org/ixmp
Apache License 2.0
38 stars 112 forks source link

Update RTD config #483

Closed glatterf42 closed 1 year ago

glatterf42 commented 1 year ago

Superseded by #485

As noted by Xiaoyang on Slack, it's not intuitive to choose [tests] as the extra requirement for building the docs when we also have [docs]. This PR replaces the selection hoping it works out of the box. In the same process, it renames the config file to comply with current recommendations.

How to review

PR checklist

codecov[bot] commented 1 year ago

Codecov Report

Merging #483 (8a680d1) into main (f348a6b) will not change coverage. The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #483   +/-   ##
=====================================
  Coverage   98.5%   98.5%           
=====================================
  Files         42      42           
  Lines       4489    4489           
=====================================
  Hits        4425    4425           
  Misses        64      64           
glatterf42 commented 1 year ago

As per the instructions for checking the RTD build locally, the command completed successfully. We might still be missing some packages required for building the docs by only requiring the extra [docs], but I'm not sure how to test that since the PR originates from my fork (so we cannot simply enable builds for this branch on RTD itself).

khaeru commented 1 year ago

We might still be missing some packages required for building the docs by only requiring the extra [docs], but I'm not sure how to test that since the PR originates from my fork (so we cannot simply enable builds for this branch on RTD itself).

RTD did not originally support building docs for PRs, but has recently added this feature: https://docs.readthedocs.io/en/latest/guides/pull-requests.html. We could turn this on, but should first consider security implications, which I haven't had time to do. (For example: code examples in the documentation or code in Sphinx conf.py can be run while the Sphinx build occurs. Could a user abuse this to, e.g. attempt to DDoS our or someone else's public-facing servers?)

So some options would be:

  1. Cautiously or temporarily (just for this PR) enable that.
  2. Re-open the PR with the same commit pushed to a branch in the main repo.

If (2), then see https://github.com/iiasa/ixmp/issues/484#issuecomment-1606883266 and the comment linked from there.

glatterf42 commented 1 year ago

From these options, I would choose 2 since I don't know how to assess DDoS potential. However, I'm not sure what I can gain from the linked comment: do you suggest to merge a PR with the suggested changes before opening a new one here?

khaeru commented 1 year ago

However, I'm not sure what I can gain from the linked comment: do you suggest to merge a PR with the suggested changes before opening a new one here?

I meant that, if/when you push to a new branch in the main repo and open a new PR, it could be convenient to add 1 more commit adding the 1 suggested line, at the same time.

glatterf42 commented 1 year ago

Superseded by #485