rest2html: treat referenced wrapped images in base document as inlined#1935
rest2html: treat referenced wrapped images in base document as inlined#1935jdknight wants to merge 1 commit intogithub:masterfrom
Conversation
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
!unstale |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
!unstale |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
!unstale |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
!unstale |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
!unstale |
There was a problem hiding this comment.
Pull request overview
This PR updates the ReST-to-HTML translation to avoid emitting trailing whitespace after an image when the image is wrapped in a reference that is a direct child of the document, preventing GitHub’s link decoration from rendering across the whitespace after the image.
Changes:
- Strip the trailing newline emitted for a referenced top-level image so it behaves like an inline image in GitHub HTML output.
Show a summary per file
| File | Description |
|---|---|
| lib/github/commands/rest2html | Adjusts depart_image output to remove trailing newline for document-level referenced images to avoid decorative underline/whitespace artifacts. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
| # treat images held in a reference on the base document as inlined | ||
| # images; this is to help avoid rendering a reference's decorative | ||
| # line for the spacing after an image | ||
| if (isinstance(node.parent, nodes.reference) | ||
| and isinstance(node.parent.parent, nodes.document)): | ||
| self.body.append(self.body.pop().rstrip('\n')) |
There was a problem hiding this comment.
This change adjusts HTML whitespace semantics in a subtle way, but the existing markup tests normalize/strip blank text nodes via Nokogiri (noblanks), so they likely won’t fail if the newline/whitespace regresses. Consider adding a targeted regression test that asserts the raw HTML output for a top-level .. image:: with :target: does not contain whitespace between the <img> and the closing </a> (string-level assertion, not DOM-equality).
docutils will only add newlines around images it believes are inlined. For images held in references, it checks the parent of the reference if its a `TextElement` to consider it inlined. Since a document is not a `TextElement` type, it will wrap an image with newlines. For GitHub output, this is not desired and will result in the extra whitespace being rendered with a reference's decorative line. To avoid this, always strip any appended suffixes for images in this scenario. Signed-off-by: James Knight <git@jdknight.me>
zkoppert
force-pushed
the
strip-suffix-on-referenced-imgs-at-doc-root
branch
from
600f67a to
ee00347
Compare
|
Hey @jdknight — thanks for sticking with this PR, and sorry it took so long to get eyes on it. I rebased your branch onto master to get CI running (it was 46 commits behind), and all checks pass across Ruby 3.2/3.3/3.4. The fix itself looks good — we reviewed it thoroughly and the logic is sound. One thing we'd like to see before merging: a regression test. The existing test suite uses Nokogiri with Something like an RST fixture with a |
docutils will only add newlines around images it believes are inlined. For images held in references, it checks the parent of the reference if its a
TextElementto consider it inlined. Since a document is not aTextElementtype, it will wrap an image with newlines. For GitHub output, this is not desired and will result in the extra whitespace being rendered with a reference's decorative line. To avoid this, always strip any appended suffixes for images in this scenario.There are a various GitHub projects which reveal the issue. For example, Sphinx's
README.rstshows this issue:With the changes made in this merge request, the following shows a rendering of HTML before and after the change: