Skip to content
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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

alphasentaurii
Copy link
Contributor

  • 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

@braingram
Copy link
Contributor

For the pre commit errors one option is to install and run pre-commit locally (as it will autofix many errors):

pip install pre-commit
pre-commit run --all-files

@braingram
Copy link
Contributor

braingram commented Dec 4, 2024

I think for the test failures updating this fixture might be required:

def docs_schema_ids():

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 sphinx-asdf managed links. I think practically this is only used by rad at the moment.

For example, rad currently links back to the asdf-standard docs with an asdf_schema_reference_mappings:
https://github.com/spacetelescope/rad/blob/3d43b917b31b3d8f0301ae95b5ae1dca3e9eedbf/docs/conf.py#L200
This allows schema uris in the rad schemas that start with http://stsci.edu/schemas/asdf/ to generate valid links back to the schema in the standard. For example the file_date schema links to time-1.1.0. I think that link still works with this PR:
https://asdf-standard--446.org.readthedocs.build/en/446/generated/stsci.edu/asdf/time/time-1.1.0.html
so that makes me think the test fixture needs updating to see the new docs layout.

@alphasentaurii
Copy link
Contributor Author

I think for the test failures updating this fixture might be required:

def docs_schema_ids():

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 sphinx-asdf managed links. I think practically this is only used by rad at the moment.

For example, rad currently links back to the asdf-standard docs with an asdf_schema_reference_mappings: https://github.com/spacetelescope/rad/blob/3d43b917b31b3d8f0301ae95b5ae1dca3e9eedbf/docs/conf.py#L200 This allows schema uris in the rad schemas that start with http://stsci.edu/schemas/asdf/ to generate valid links back to the schema in the standard. For example the file_date schema links to time-1.1.0. I think that link still works with this PR: https://asdf-standard--446.org.readthedocs.build/en/446/generated/stsci.edu/asdf/time/time-1.1.0.html so that makes me think the test fixture needs updating to see the new docs layout.

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.

@braingram

This comment was marked as resolved.

@alphasentaurii
Copy link
Contributor Author

Is there a way to configure the syntax highlighting for the new theme to provide more colors for the yaml?

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)

Copy link
Contributor

@braingram braingram left a 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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>
Copy link
Contributor

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).

Copy link
Contributor

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"]
Copy link
Contributor

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
Copy link
Contributor

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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants