docs: build relative path to preview examples #937
Conversation
|
Documentation. Coverage Reports: |
spliffone
force-pushed
the
docs/build-relative-examples-path
branch
2 times, most recently
from
4a92e78 to
6d544ec
Compare
spike-rabbit
left a comment
spike-rabbit
left a comment
There was a problem hiding this comment.
This seems to work, but please remove EXAMPLES_BASE env var from the build-and-test.yaml
spliffone
force-pushed
the
docs/build-relative-examples-path
branch
from
6d544ec to
b8495d6
Compare
WalkthroughAdds page-aware relative path resolution to the element-docs markdown extension: a new Changes
Sequence DiagramsequenceDiagram
participant MkDocs as mkdocs.yml (config)
participant Ext as ElementDocsExtension
participant Prep as ElementExamplePreProcessor
participant FS as Filesystem/Path
MkDocs->>Ext: load `markdown_extensions.element-docs.md_file`
Ext->>Ext: extract page URL from `md_file`
Ext->>Prep: initialize preprocessor with `examples_base` + page
Prep->>FS: compute relative path (use page depth if no URL scheme)
FS-->>Prep: resolved examples_base path
Prep->>Ext: process markdown includes with resolved paths
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mkdocs.yml(1 hunks)projects/element-docs-builder/md_extension_element_docs/extension.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
projects/element-docs-builder/md_extension_element_docs/extension.py (1)
projects/element-docs-builder/md_extension_element_docs_composer/extension.py (1)
extendMarkdown(20-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
mkdocs.yml (1)
236-239: Using!relativehere is fine, but it ties you to MkDocs ≥ 1.5
md_file: !relativeinmarkdown_extensionsis a valid use of MkDocs’!relativetag and gives the extension access to page-local context.(mkdocs.org)
However, it implicitly requires MkDocs 1.5+ and thatelement-docsis only used in that environment. If older MkDocs versions must remain supported, this will fail at config load time; otherwise, consider documenting the minimum MkDocs version in CONTRIBUTING/README so users understand the new requirement.To double‑check compatibility across MkDocs versions you support, please confirm against the version matrix you target (e.g., CI jobs) and adjust docs or pin MkDocs accordingly.
spliffone
force-pushed
the
docs/build-relative-examples-path
branch
from
b8495d6 to
cae9512
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
projects/element-docs-builder/md_extension_element_docs/extension.py (1)
4-4: Remove unusedPathimport.The
Pathclass is imported but never used in the code. The relative path logic correctly uses string manipulation instead.Apply this diff to remove the unused import:
-from pathlib import Path from typing import cast
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mkdocs.yml(1 hunks)projects/element-docs-builder/md_extension_element_docs/extension.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mkdocs.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
projects/element-docs-builder/md_extension_element_docs/extension.py (1)
144-147: Config updates look good.The
md_fileconfig description accurately reflects its purpose as a!relativehelper for accessing the MkDocs config.
| url = urlparse(self.examples_base) | ||
| # If no scheme, we assume we need to build a relative path to the examples | ||
| if url.scheme == '': | ||
| # Number of non-empty segments in /foo/bar/ → 2, used as "../" depth | ||
| depth = len([s for s in self.page.strip('/').split('/') if s]) | ||
| rel_prefix = '../' * depth | ||
| self.examples_base = f'{rel_prefix}{self.examples_base.lstrip("/")}' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for examples_base configuration in YAML files and check for relative paths
# Find all YAML files and search for examples_base
rg -n "examples_base" --type yaml -A 2 -B 2
# Also search in any Python config files
rg -n "examples_base" --type py -A 2 -B 2 | head -50Repository: siemens/element
Length of output: 4506
Based on my verification, I can now provide the rewritten review comment. The original concern is valid and confirmed. The code comments at lines 130-131 explicitly mention the case where examples_base could be ../../../demo/index.html on a server, but the code doesn't normalize this input before prepending additional ../ segments based on page depth.
Relative path double-prepending is possible with the current logic.
The code at lines 104–110 will prepend additional ../ segments to any examples_base without a URL scheme. However, if examples_base is already a relative path (e.g., ../../../demo/index.html, as mentioned in the comments at lines 130–131), this results in double-prepending. For example, a page at /docs/reference/foo/ (depth 3) would transform ../../../demo/index.html into ../../../../../../../../../demo/index.html, potentially overshooting the target.
To fix this, either:
- Normalize
examples_baseby removing leading../before applying the page-relative prefix, or - Document that
examples_basemust be an absolute path (e.g.,/demo/index.html) or a bare relative path without../(e.g.,demo/index.html), or - Use a proper path normalization library to resolve the final path correctly.
🤖 Prompt for AI Agents
In projects/element-docs-builder/md_extension_element_docs/extension.py around
lines 104-110, the code prepends a page-relative "../" prefix to examples_base
without handling cases where examples_base already starts with "../", causing
double-prepending; fix it by normalizing examples_base before prefixing—either
strip any leading "../" segments from self.examples_base (or convert leading
"./" to none) then prepend rel_prefix, or resolve the final path using a POSIX
path normalization/join (e.g., posixpath.join + posixpath.normpath) so multiple
"../" segments collapse correctly and the resulting relative path is normalized.
| # Extract current page path | ||
| md_file_cfg = self.config.get('md_file')[0] | ||
| mkdocs_config = getattr(md_file_cfg, 'config', None) | ||
| current_page = getattr(mkdocs_config, '_current_page', None) | ||
| page = getattr(current_page, 'abs_url', '') or '' | ||
| if hasattr(current_page, 'abs_url'): | ||
| page = current_page.abs_url | ||
| md.preprocessors.register(ElementExamplePreProcessor(self.config.get('examples_base')[0], page, md), 'element_example', 10) |
There was a problem hiding this comment.
Fix potential IndexError and remove redundant logic.
Two issues in the page extraction logic:
-
Line 153 could raise
IndexErrorifmd_fileis configured as an empty list. While unlikely in normal usage, defensive programming would check the list length first. -
Lines 156-158 contain redundant logic. Line 156 already extracts
abs_urlwith a fallback to empty string usinggetattr. Lines 157-158 then repeat the same extraction withhasattr, overwriting the result. The second check is unnecessary.
Apply this diff to fix both issues:
# Extract current page path
-md_file_cfg = self.config.get('md_file')[0]
+md_file_list = self.config.get('md_file', [''])
+md_file_cfg = md_file_list[0] if md_file_list else ''
mkdocs_config = getattr(md_file_cfg, 'config', None)
current_page = getattr(mkdocs_config, '_current_page', None)
page = getattr(current_page, 'abs_url', '') or ''
-if hasattr(current_page, 'abs_url'):
- page = current_page.abs_url
md.preprocessors.register(ElementExamplePreProcessor(self.config.get('examples_base')[0], page, md), 'element_example', 10)🤖 Prompt for AI Agents
In projects/element-docs-builder/md_extension_element_docs/extension.py around
lines 152 to 159, the code currently indexes self.config.get('md_file')[0]
without checking the list length and redundantly re-reads abs_url; change it to
first safely retrieve the md_file list, verify it contains at least one element
and bail or use a safe default if empty, then use mkdocs_config =
getattr(md_file_cfg, 'config', None) and current_page = getattr(mkdocs_config,
'_current_page', None) and set page = getattr(current_page, 'abs_url', '')
(remove the subsequent hasattr block), and finally register the preprocessor
using the safe page variable; ensure you do not assume md_file is non-empty to
avoid IndexError.
|
|
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.