-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add function to generate the configuration file #10148
Conversation
A quick grep shows you where they are defined:
Re: Replacing |
This looks very good! My own sense would be to try to aim to have it auto-included in the documentation. If that's done at the end, we don't even have to worry about importing astropy modules... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I have this stupid little script here: https://github.com/astropy/ci-helpers/blob/master/utils/import_submodules.py that maybe of use here, too. (I use it in a WIP PR: https://github.com/astropy/astropy/pull/7952/files#diff-b91f3d5bd63fcd17221b267e851608e8R94) |
@bsipocz - thanks, I just pushed something using |
So I added a very simple Sphinx extension to include directly the generated config in the docs: Should this extension go in sphinx-astropy ? |
This extension only works for the core library, right? Should it just stay here like it is already in the PR? |
Fine by me 👍 |
I see that we might want to use something like this for astroquery where we have configs for all subpackages, but keeping it in core for now sounds reasonable. |
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.
This looks very nice! I'll leave the question about where this should live to @astrofrog, but think starting in astropy certainly makes sense. Only one suggestion for making the creation a bit more elegant.
This comment has been minimized.
This comment has been minimized.
cad1fbd
to
a0c8eb9
Compare
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.
So, this needs astropy/sphinx-astropy#30 for the HTML build to work properly, right?
Maybe add an entry to releasing procedures to update the astropy.cfg
with this function before a release, near the instruction to update leap seconds table?
Overall, LGTM! I think with this in, the risk of the default astropy.cfg
falling behind will decrease significantly. I also really appreciate the extra work that went into make the function useful for affiliated packages down the road. Thanks!
I'll let the sphinx-astropy
dependency question get sorted out first before approving.
@@ -1,144 +1,169 @@ | |||
# -*- coding: utf-8 -*- |
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.
coding
is removed. Does it affect anything?
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.
It was mostly an issue with Python 2, but with Python 3 it should be ok:
If no encoding declaration is found, the default encoding is UTF-8.
https://docs.python.org/3/reference/lexical_analysis.html#encoding-declarations
astropy/config/configuration.py
Outdated
for mod in pkgutil.walk_packages(path=package.__path__, | ||
prefix=package.__name__ + '.'): | ||
|
||
if mod.module_finder.path.endswith('tests'): |
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.
Just in case?
if mod.module_finder.path.endswith('tests'): | |
if mod.module_finder.path.endswith(('tests', 'test')): |
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.
Done.
# module name, but not if this is the root package... | ||
if item.module != pkgname: | ||
modname = item.module.replace(f'{pkgname}.', '') | ||
fp.write(f"[{modname}]\n\n") |
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.
Is it safer to use os.linesep
instead of "\n"
?
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.
open
takes care of this by default:
- On input, if newline is None, universal newlines mode is
enabled. Lines in the input can end in '\n', '\r', or '\r\n', and
these are translated into '\n' before being returned to the
caller. [...]- On output, if newline is None, any '\n' characters written are
translated to the system default line separator, os.linesep.
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.
Ah, that's so good to know!
with open(outfile) as fp: | ||
conf2 = fp.read() | ||
|
||
for c in (conf, conf2): |
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.
Maybe a comment to explain why these specific lines are selected for testing? Were they arbitrarily selected or were there reasons?
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.
Just a random selection 😄
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.
Added a comment.
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.
LGTM! Since this depends on new feature in sphinx-astropy
, I'll let @bsipocz and/or @astrofrog figure out when to merge.
Thanks!
Yes, merging this should wait until the sphinx-astropy release is out. Alternatively we can add the dev version as dependency in the meantime and wait with the release until the end of the RC testing period. Either would work equally well for me. There is one more thing, we'll need to add a new minimum sphinx-astropy version requirement, otherwise the docs build will fail with the missing directive error. |
I've just released sphinx-astropy 1.3! Can you try adding that as a minimum requirement if we are going to be relying on this new plugin? (and this will also trigger a CI build) |
Thank you @saimn, this looks awesome now! |
Ref #10146. This was fun and quick enough to do, but I had to hard-code the list of modules to import, not sure if there is a more clever (and simple) solution ?
Result: https://gist.github.com/saimn/b01966dbc6cef2c8c849ac4b933e84d3
This is almost the same as
astropy.cfg
, the order of subpackages is slightly different (alphabetical with the function here), and the only thing missing is the manual subsections like### CORE DATA STRUCTURES AND TRANSFORMATIONS
.Next steps:
astropy.cfg
occasionally, or replace it ?astropy.cfg
is not so easy since it is used as a template for the default config file, which is checking while importing astropy. But the solution here takes a bit of time since it needs to import all modules with config items...EDIT: Fix #10146