-
Notifications
You must be signed in to change notification settings - Fork 30
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
docs/asdf-website-changes #446
base: main
Are you sure you want to change the base?
Conversation
alphasentaurii
commented
Dec 3, 2024
- Use furo theme for consistency with parent asdf website
- Remove intro page (contents moved to asdf website overview page: https://asdf-website.readthedocs.io/en/latest/overview.html)
- Improved toc tree / page hierarchy
- added new logo
- configuration minor fixes
- allow livehtml preview during doc changes
For the pre commit errors one option is to install and run pre-commit locally (as it will autofix many errors):
|
I think for the test failures updating this fixture might be required: asdf-standard/tests/conftest.py Line 94 in 35a6132
or changing the docs layout to match what's on main (although I'm in favor of updating the fixture). One thing to consider is that external packages can link to the docs via For example, rad currently links back to the |
Yes - the "generated" file links are all the same as before. The only true change is the structure of the pages. I did have to update the test_docs.py test to look for asdf.rst since that is a new file (its contents were moved from schemas/index.rst). Most of the changes I made were for the sake of improving the TOC tree and breaking up sections into separate pages where it made sense. In retrospect I'm not sure it was worth the trouble but now all the tests are passing so we'll go with it. I'll be on standby if some obscure link has been broken but as far as I can tell everything is in working order. |
This comment was marked as resolved.
This comment was marked as resolved.
Yes, that can be configured via sphinx's pygments_style setting in conf.py and is also fully customizable. I'll look at the options and see if I can find one more suitable (i.e. similar to the sphinx theme) |
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.
Thanks for putting this together. Here's a round of comments. Overall the changes look good to me with a few suggestions/questions about the:
- heading changes
- doc reorganization
I assume we'll want to make similar changes in other asdf-format repos to at least keep a consistent theme. It would be helpful to split up those PRs into smaller PRs with:
- theme changes
- reorganization
- content changes
I worry with this PR that I missed some of the content changes due to the reorganization.
@@ -0,0 +1,260 @@ | |||
.. _extending-asdf: | |||
|
|||
Schema Design and Editing |
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.
Schema Design and Editing | |
Schema Design and Editing |
I am in favor of the old name:
https://asdf-standard.readthedocs.io/en/1.1.1/schemas.html#designing-a-new-tag-and-schema
"Designing a new tag and schema" since it makes more of a distinction between the "tag" and "schema".
Versioning Conventions <versioning.rst> | ||
ASDF Schemas <asdf-schemas.rst> | ||
Known Limits <known_limits.rst> | ||
ASDF in FITS <asdf_in_fits.rst> |
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.
On main this shows up as "Appendix A: Embedding ASDF in FITS" in the TOC. With this PR it loses the "Appendix A" part. I think we should keep that as an appendix to avoid confusion about this being something we generally support (which we don't).
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'd be in favor of removing it from the top level TOC if that's easier.
@@ -11,7 +11,8 @@ | |||
|
|||
SCHEMAS_PATH = RESOURCES_PATH / "schemas" / "stsci.edu" / "asdf" | |||
DOCS_PATH = ROOT_PATH / "docs" / "source" | |||
DOCS_SCHEMAS_PATH = DOCS_PATH / "schemas" | |||
DOCS_SCHEMAS_PATH = DOCS_PATH | |||
DOCS_SCHEMAS_LIST = ["asdf.rst", "astronomy.rst", "core.rst", "legacy.rst"] |
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.
What's the need for moving these out of a "schemas" subdirectory? I'm in favor of keeping to old structure if it doesn't cause complications (since it's easier to tell what portions of the docs are describing schema resources).
@@ -0,0 +1,63 @@ | |||
.. _asdf-schemas: | |||
|
|||
ASDF Schemas |
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.
The manifests aren't schemas. I'm in favor of keeping the old heading:
"ASDF Standard Resources"