Skip to content

Read page descriptions on older Docutils releases - #156

Merged
jacobtomlinson merged 1 commit into
NVIDIA:mainfrom
MaxFreedomPollard:fix/legacy-docutils-page-descriptions
Sep 21, 2026
Merged

Read page descriptions on older Docutils releases#156
jacobtomlinson merged 1 commit into
NVIDIA:mainfrom
MaxFreedomPollard:fix/legacy-docutils-page-descriptions

Conversation

@MaxFreedomPollard

@MaxFreedomPollard MaxFreedomPollard commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

On Docutils releases before 0.18, docutils.nodes.meta is not available. Reading a page description raises AttributeError, so llms.txt falls back to page content instead of using the authored html_meta description.

Use the same tag-name fallback already used by docref, and add a regression test for the missing node class. The test helper also works with older Docutils so the existing fresh-build and incremental-build tests can exercise that combination.

Validation:

  • Sphinx 5.3.0 / Docutils 0.17.1: nine metadata tests fail before the fix; all 13 metadata and content-fallback tests pass after it.
  • Current dependencies: all 465 tests pass.
  • Pre-commit checks pass.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA/sphinx-llm/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8ed6dbc7-eedc-40ef-9c2e-547209a60a6e

📥 Commits

Reviewing files that changed from the base of the PR and between 28d1e1a and 046e363.

📒 Files selected for processing (2)
  • src/sphinx_llm/tests/test_txt.py
  • src/sphinx_llm/txt.py

Included review availability: Your plan provides up to 12 included reviews per hour; 9 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved page description metadata extraction for broader compatibility with different Docutils environments.
    • Added coverage to ensure descriptions remain available when the standard metadata node reference is unavailable.

Walkthrough

The metadata description lookup now supports Docutils environments without docutils.nodes.meta. Tests verify that HTML descriptions remain available through tag-name matching.

Changes

Metadata extraction compatibility

Layer / File(s) Summary
Metadata fallback and regression coverage
src/sphinx_llm/txt.py, src/sphinx_llm/tests/test_txt.py		 get_page_description falls back to matching nodes with the meta tag name. Tests verify description extraction when docutils.nodes.meta is unavailable.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: kartben

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: supporting page descriptions on older Docutils releases.
Description check ✅ Passed The description directly explains the Docutils compatibility issue, the fallback implementation, regression coverage, and validation results.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

codecov-commenter commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.17%. Comparing base (21647d2) to head (046e363).
⚠️ Report is 32 commits behind head on main.

Files with missing lines Patch % Lines
src/sphinx_llm/tests/test_txt.py 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##             main     #156       +/-   ##
===========================================
+ Coverage   81.89%   95.17%   +13.28%     
===========================================
  Files           4       13        +9     
  Lines         624     3337     +2713     
  Branches       87      314      +227     
===========================================
+ Hits          511     3176     +2665     
- Misses         84      107       +23     
- Partials       29       54       +25
Flag Coverage Δ
unittests 95.17% <92.85%> (+13.28%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jacobtomlinson
jacobtomlinson reviewed Sep 21, 2026
View reviewed changes

@jacobtomlinson jacobtomlinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution!

The oldest sphinx version currently supported here is v5.1.0 which pins docutils>=0.14,<0.20. We don't explicitly test different docutils versions in CI, so the Sphinx 5.1 job will likely be pulling in docutils 0.19.x.

I think this highlights that we should add a mindeps CI job that tests our package floor for all dependencies including transients. I'm not comfortable merging this PR until we have a solid mindeps CI job. I'll start another PR to add that in.

@jacobtomlinson

jacobtomlinson commented Sep 21, 2026

Copy link
Copy Markdown
Member

I've prepared #158 to test this in CI. It looks like my codex came to the same conclusion about fixing this meta node issue. I think both implementations have value so I'm going to merge this PR and then immediately merge #158 as a follow up.

@jacobtomlinson
jacobtomlinson merged commit 0798b5b into NVIDIA:main Sep 21, 2026
19 checks passed
@jacobtomlinson-bot jacobtomlinson-bot mentioned this pull request Sep 21, 2026
Merged
@jacobtomlinson jacobtomlinson added the bug Something isn't working label Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobtomlinson jacobtomlinson jacobtomlinson left review comments

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MaxFreedomPollard @codecov-commenter @jacobtomlinson