Skip to content

Render usage.sh with sphinx-gallery #590

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

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

Render usage.sh with sphinx-gallery #590

wants to merge 2 commits into from

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented May 8, 2025

Should make things easier to maintain. We now only have one file and no linking between two. sphinx-gallery does not run the file for us. We still run it in the tox job.


📚 Documentation preview 📚: https://metatrain--590.org.readthedocs.build/en/590/

@Copilot Copilot AI review requested due to automatic review settings May 8, 2025 14:58
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request streamlines the usage documentation by consolidating the primary example to a single shell script and updating the Sphinx Gallery configuration accordingly.

  • Removed the separate reST file for usage and migrated its content to usage.sh.
  • Updated the Sphinx Gallery configuration to include the basic_usage examples.
  • Adjusted navigation links in the documentation index to point to the new usage file.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

File Description
examples/basic_usage/usage.sh Updated to include usage instructions and Sphinx Gallery cell markers; note the removal of the original shebang.
docs/src/getting-started/usage.rst Removed the file in favor of the consolidated usage.sh example.
docs/src/getting-started/index.rst Updated link reference to point to the new usage file.
docs/generate_examples/conf.py Modified filename/ignore patterns and added basic_usage to examples and gallery directories.
Comments suppressed due to low confidence (1)

examples/basic_usage/usage.sh:1

  • The file no longer includes a valid shebang line, which may cause issues when running it in the tox job; consider adding a proper shebang (e.g., #!/bin/bash) at the top of the file.
# .. _label_basic_usage:

@PicoCentauri PicoCentauri force-pushed the sh-gallery branch 2 times, most recently from 27a49a6 to 57258cd Compare May 8, 2025 15:02
@PicoCentauri
Copy link
Contributor Author

PicoCentauri commented May 8, 2025

@PicoCentauri PicoCentauri requested a review from Luthaf May 8, 2025 15:15
@Luthaf
Copy link
Member

Luthaf commented May 8, 2025

I don't understand why is this a bash script in the first place? Are we actually testing something by running it?

Otherwise I would just transform this into a normal rst file and not have the added complexity =)

@PicoCentauri
Copy link
Contributor Author

Yes we run it!

I think a pure RST file is very very dangerous for us. We change the API very rapidly and most of time nobody cares about the docs. If we just have a pure rst file it will be broken within a couple of days.

@Luthaf
Copy link
Member

Luthaf commented May 8, 2025

I mean, this is only smoke-testing the CLI API, right? I though we also had CLI API tests in Python elsewhere

@PicoCentauri
Copy link
Contributor Author

PicoCentauri commented May 8, 2025

Yes, it is a smoke test.

metatrain/tox.ini

Lines 155 to 158 in 976a66d

bash -c "set -e && cd {toxinidir}/examples/basic_usage && bash usage.sh"
bash -c "set -e && cd {toxinidir}/examples/ase && bash train.sh"
bash -c "set -e && cd {toxinidir}/examples/programmatic/llpr && bash train.sh"
bash -c "set -e && cd {toxinidir}/examples/zbl && bash train.sh"

But better a smoke test than no test. If Sphinx-Gallery allows us to nicely render a .sh file that we run, I think this is better than just having a .rst file that could break and is not run at all. The rendered result for the user is the same.

Copy link
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

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

Looks good, one other small nitpick on tox and this is then good to go

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