Skip to content

Docs scripting fixing#9729

Closed
oharboe wants to merge 17 commits into
The-OpenROAD-Project:masterfrom
Pinata-Consulting:docs-scripting-fixing
Closed

Docs scripting fixing#9729
oharboe wants to merge 17 commits into
The-OpenROAD-Project:masterfrom
Pinata-Consulting:docs-scripting-fixing

Conversation

@oharboe
Copy link
Copy Markdown
Collaborator

@oharboe oharboe commented Mar 11, 2026

No description provided.

@oharboe oharboe requested a review from maliberty March 11, 2026 20:02
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request significantly improves the documentation generation scripts by adding a comprehensive test suite with pytest and pytest-cov, and making the scripts more robust by improving error handling and fixing several potential bugs. The changes from using assert and exit() to proper ValueError exceptions with informative messages are excellent. I've found a couple of minor areas for improvement to further increase robustness and code clarity.

Comment thread docs/src/scripts/extract_utils.py
Comment thread docs/src/scripts/md_roff_compat.py Outdated
oharboe and others added 14 commits March 11, 2026 21:15
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
…ompat

The assertion error gave no hint as to what caused the mismatch or how
to fix it. Replace with a ValueError that names the file, labels each
count with what it measures, highlights only the mismatched entries, and
lists the two common root causes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
exit() inside a library function bypasses all callers and produces no
error message. Raise ValueError with context instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
assert is disabled by python -O and raises AssertionError, which gives
no context. Use an explicit ValueError with the bad value included.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
open() without encoding uses the platform default, which differs across
locales and OSes. Force utf-8 for reproducible behaviour.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
When a section has no '#' character, find() returns -1 and
a[-1:] silently slices the last character instead of producing
an empty string. Make the no-match case explicit.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
If no level-2 header follows the last level-3 header the list
comprehension is empty and [0] raises an IndexError with no context.
Raise a ValueError with the offending header name instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Repeated str += in a loop copies the growing string on every iteration.
Collect lines into a list and join once at the end.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
44 tests covering extract_headers, extract_tcl_command/code,
extract_description, extract_arguments, extract_tables, parse_switch,
check_function_signatures, man2_translate, and man3_translate.

man2/man3 tests use unittest.mock to avoid filesystem side-effects;
the full suite runs in under 0.1 s.

Coverage: extract_utils 86%, md_roff_compat 80%.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Add 'make test' target (runs pytest from docs/) and pytest.ini
configuring testpaths, python_files, and --cov defaults.
Add pytest and pytest-cov to requirements.txt so CI installs them
alongside the existing Sphinx dependencies.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Explains what each test file covers, how to run locally, and how
to wire 'make test' into CI before the existing 'make preprocess' step.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Make 'preprocess' depend on 'test' so the unit tests always run first
when CI calls 'make preprocess -C docs'. A broken parser will fail fast
before attempting to generate man-pages.

Update TESTING.md to reflect the new dependency.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
@oharboe oharboe force-pushed the docs-scripting-fixing branch from e48ca4d to f5c2f74 Compare March 11, 2026 20:16
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

oharboe and others added 2 commits March 11, 2026 21:22
{k: v for k, v in counts.items()} is just counts.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Accessing level3[-1] in the ValueError message would raise an IndexError
if the document has no ### sections. Return [], [] early in that case.
Add a test to cover the new branch.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

CI uses psf/black@stable (26.x); locally 25.x was installed, masking
a formatting difference. Update to match.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
@github-actions
Copy link
Copy Markdown
Contributor

clang-tidy review says "All clean, LGTM! 👍"

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.

1 participant