Repository navigation
Add extension to include generated config in the docs - #30
Conversation
| required_arguments = 1 | ||
|
|
||
| def run(self): | ||
| from astropy.config import generate_config |
There was a problem hiding this comment.
isn't the idea to bring generate_config() over here, too?
There was a problem hiding this comment.
All the config related stuff is in astropy, and this new function could be used there to generate the config file when needed, it is not only for the docs.
There was a problem hiding this comment.
generate_config should stay in core library, not here.
There was a problem hiding this comment.
ok, fair enough, it was my misunderstanding. Having to require astropy as a dependency for this on the other hand seems a bit extreme when the extension has such a generic name (maybe rename sphinx_astropy.ext.generate_astropy_config?)
There was a problem hiding this comment.
It would work for other packages, e.g. astroquery. And sphinx-astropy is an astropy thing anyway.
There was a problem hiding this comment.
I meant the astropy in the name to mean the astropy type of config system, that astroquery also uses.
Either way, it's not a strong preference, but usually folks are arguing that both our sphinx and pytest stuff is widely generic and complained in the past on reliance on reliance on astropy (thus they got moved out from the helpers).
Anyway, it needs to be @astrofrog who reviews and merge this.
There was a problem hiding this comment.
I think astropy is not needed in the extension name since the extension itself indeed already has 'astropy' in the name.
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
This looks good to me - ideally it should be tested but I think we have a circular issue here that ideally we need the astropy function to exist. So this looks good to me, but a test should be added once astropy 4.1 is out.
| required_arguments = 1 | ||
|
|
||
| def run(self): | ||
| from astropy.config import generate_config |
There was a problem hiding this comment.
I think astropy is not needed in the extension name since the extension itself indeed already has 'astropy' in the name.
|
Thanks @astrofrog, and agreed for the test. |
4 participants
Ref astropy/astropy#10148
Not sure how to test this, since this would require to install astropy here...