-
-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should ask RTD for access to their PR builder feature for this repo, but that's a different issue.
Not sure how to test this patch before merge...
python: | ||
version: 3.5 | ||
version: 3.7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason to not use Python 3.7?
remove_file('.rtd-environment.yml') | ||
remove_file('readthedocs.yml') | ||
remove_file('rtd-pip-requirements') | ||
remove_file('.readthedocs.yml') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the typo fix for #429.
fail_on_warning: false | ||
|
||
# Set the version of Python and requirements required to build your docs | ||
# NOTE: This does not follow {{ cookiecutter.minimum_python_version }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if one should build doc from minimum Python version, so I just hardcode it to Python 3.7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, this is just a template, maintainers are expected to look under the hood, at least a little bit, and then your comment clears up the situation.
This looks good to me. The comments are rhetorical ones really, I suppose I'm happy to merge this as is. |
What if we declared an extras requires for docs in setup.cfg and put Sphinx-astropy there, then used that in the RTD config? Would be more consistent with the core package than using a rtd requirements file? |
Just to be clear I’m ok with this being merged and happy to do a follow up PR |
The core still uses a RTD requirements file on top of the I agree that using |
Feel free to merge if you are okay with this. I try not to merge my own PRs. 😉 |
system_packages: true | ||
install: | ||
- requirements: requirements.txt | ||
- method: setuptools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why setuptools
and not pip
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To run python setup.py install
. I think @astrofrog already brought it up above and said the pip
thing can be a separate PR.
system_packages: true | ||
install: | ||
- requirements: rtd-pip-requirements | ||
- method: setuptools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why setuptools and not pip?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same answer as above.
@@ -0,0 +1,5 @@ | |||
Cython |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we move this file to docs/requirements.txt
or docs/rtd-requirements.txt
? I feel like we don't need another file in the root of the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can move it as long as there is a consensus. I feel people have been flip-flopping where that file should go throughout the years and we move it back and forth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a preference, but picking one and sticking with it in all the repos would be nice. So far astropy and photutils have docs/rtd_requirements.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I moved and renamed it. Also squashed commits.
Move doc requirement files to docs/requirements.txt.
On the |
@Cadair , I am not using |
No, I was responding to @astrofrog's comments. |
Superseded by #438 |
Fix #429
Fix #431