Add the sphinx-alt-text-validator tool to VCS install it in each API repo #2445
Conversation
| return alt_exists or nofigs_exists | ||
|
|
||
|
|
||
| def validate_image(file_path: str) -> tuple[str, list[str]]: |
There was a problem hiding this comment.
Same function but removing this conditional:
if file_path in ALLOWLIST_MISSING_ALT_TEXT:
return [file_path, []]Now the function gets the files filtered from __init__.py so no need to verify any allowlist anymore
| parser = argparse.ArgumentParser(prog="verify_images.py") | ||
| parser.add_argument("-f", "--folder", required=True) | ||
| parser.add_argument("-s", "--skip", nargs="+") | ||
| args = parser.parse_args() | ||
|
|
||
| skip_list = args.skip if args.skip is not None else [] | ||
| files = glob.glob(f"{args.folder}/**/*.py", recursive=True) | ||
| filtered_files = [file for file in files if file not in skip_list] |
There was a problem hiding this comment.
This is the only new code added in this PR
There was a problem hiding this comment.
Great job! I did a quick test and seems to work well :)
When this is merged to main, make sure to pin to the commit when installing in other repos. This means we can make changes to the script without worrying about accidentally breaking their CI.
pip install -e "git+https://github.com/Qiskit/documentation.git@<COMMIT-HASH>#egg=sphinx-alt-text-validator&subdirectory=scripts/image-tester"
Co-authored-by: Frank Harkins <frankharkins@hotmail.co.uk>
There was a problem hiding this comment.
Nice work! Can you please add in a follow up a simple Pytest test for validate_image? Now that it's a public API, it would be good to ensure we don't break it in the future.
- You can reuse the Pytest infrastructure we have for nb-tester. I'd probably rename the Tox env to be something like
tests - You'll probably want to change
validate_imageto be a pure function and have the text be passed in directly rather than using Pathlib
| parser.add_argument("-s", "--skip", nargs="+") | ||
| args = parser.parse_args() | ||
|
|
||
| skip_list = args.skip or [] |
There was a problem hiding this comment.
Nit: using a set will be faster when the skip list is larger
This PR creates tests for the `sphinx-alt-text-validator` and recovers the tests of the `nb-tester` in CI. See #2445 (review) and #2425 (comment) Example of the test running: https://github.com/Qiskit/documentation/actions/runs/12390997779/job/34587197281?pr=2502#step:22:1
This PR adds a tool that all the APIs should use to avoid having images without alt text. By having the script in a central repository, we could use the VCS method to pip install it everywhere instead of duplicating the same script 9 times.
The tool is currently being used in qiskit, runtime, and transpiler. Code -> https://github.com/Qiskit/qiskit/blob/main/tools/verify_images.py
The code added in this PR is the same but split into two files (
__init__.pyandverify_images.py) and with the possibility of using two new arguments to specify the folder where the linter will check their files and an argument to enumerate all the files the user wants to skip, replacing theALLOWLIST_MISSING_ALT_TEXTlist that was defined at the beginning of the script.To install and test the tool we use
pip install -e "git+https://github.com/arnaucasau/documentation.git@AC/sphinx-alt-text-validator#egg=sphinx-alt-text-validator&subdirectory=scripts/image-tester"and run
sphinx-alt-text-validator -f [FolderToCheck]Note
This PR only copies the script to the repo and adds the necessary files to make it installable from the API repos. In a follow-up, I'm planning to add some tests and maybe do a little refactoring to the verify_image.py