Improved Markdown generation in make_md.py:#1200
Merged
Arctis-Fireblight merged 2 commits intoRedot-Engine:masterfrom Feb 19, 2026
Merged
Improved Markdown generation in make_md.py:#1200Arctis-Fireblight merged 2 commits intoRedot-Engine:masterfrom
make_md.py:#1200Arctis-Fireblight merged 2 commits intoRedot-Engine:masterfrom
Conversation
- Updated anchor creation for consistency. - Added `_category_.json` generation for better doc indexing. - Introduced escaping functionality for special characters in descriptions and MD files. - Enhanced sanitization logic for class/file/operator names. - Streamlined Markdown reference links and tag escaping logic.
WalkthroughMinor doc formatting fix in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
🧹 Nitpick comments (2)
doc/tools/make_md.py (2)
1731-1731:<hr>is a void element — the</hr>closing tag is unnecessary.While not technically broken (browsers tolerate it),
<hr>is a void element per the HTML spec and should not have a closing tag. Using<hr class="...">or<hr class="..." />is more conventional.♻️ Suggested tweak
- return f'<hr class="classref-{separator_class}-separator"></hr>\n\n' + return f'<hr class="classref-{separator_class}-separator" />\n\n'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/tools/make_md.py` at line 1731, The returned HTML uses a forbidden closing tag for a void element: remove the unnecessary "</hr>" and return a proper void <hr> (e.g., '<hr class="classref-{separator_class}-separator">' or self-closing) so the string constructed where separator_class is interpolated no longer includes the "</hr>" closing tag.
1307-1307: Mutating shared definition objects withescape_tagsis fragile.Assigning back to
property_def.text(and similarlym.descriptionat lines 1245, 1356, 1404, 1450) permanently alters the shared definition object. If the object were ever rendered again (e.g., referenced from another context, or if the script logic changes), the escaping would double. Prefer escaping into a local variable instead of mutating the source.♻️ Example for the property_def.text case
- property_def.text = escape_tags(property_def.text) f.write(make_deprecated_experimental(property_def, state)) - if property_def.text is not None and property_def.text.strip() != "": - f.write(f"{format_text_block(property_def.text.strip(), property_def, state)}\n\n") + prop_text = escape_tags(property_def.text) if property_def.text is not None else None + if prop_text is not None and prop_text.strip() != "": + f.write(f"{format_text_block(prop_text.strip(), property_def, state)}\n\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/tools/make_md.py` at line 1307, Do not mutate shared definition objects by reassigning escaped strings back into them; instead compute an escaped local variable and use that for rendering. For the property case, replace "property_def.text = escape_tags(property_def.text)" with something like "escaped_text = escape_tags(property_def.text)" and use escaped_text wherever the template/render needs the escaped value; apply the same pattern for the other places that reassign (the m.description occurrences) so you never overwrite property_def.text or m.description in-place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/tools/make_md.py`:
- Line 1036: The link construction for override properties incorrectly prefixes
the anchor with "class_", causing broken anchors; in the ref assembly where
property_def, override_file and sanitize_class_name are used (the f-string
building ref), remove the "class_" prefix so the fragment becomes
`{sanitize_class_name(property_def.overrides)}_property_{property_def.name}` to
match the other anchor generation convention and ensure the link points to the
correct anchor in the target file.
- Around line 1516-1517: The escape_tags function returns malformed HTML
entities; update escape_tags (the function defined as escape_tags) to use
semicolon-terminated entities by replacing "<" with "<" and ">" with ">"
(matching the escape_md behavior) so the returned strings produce valid escaped
HTML.
- Around line 1245-1246: escape_tags is being called on optional strings (e.g.,
m.description) and will raise AttributeError when the value is None; update each
site (the shown call with m.description and the other four occurrences at the
same pattern) to only call escape_tags when the value is not None (or use a
conditional expression to set m.description = escape_tags(m.description) if
m.description is not None else None). Locate the call sites referencing
escape_tags and the optional fields coming from MethodDef.__init__ /
PropertyDef.__init__ and wrap the call in a None-check so the code preserves
None values instead of invoking escape_tags on them.
---
Nitpick comments:
In `@doc/tools/make_md.py`:
- Line 1731: The returned HTML uses a forbidden closing tag for a void element:
remove the unnecessary "</hr>" and return a proper void <hr> (e.g., '<hr
class="classref-{separator_class}-separator">' or self-closing) so the string
constructed where separator_class is interpolated no longer includes the "</hr>"
closing tag.
- Line 1307: Do not mutate shared definition objects by reassigning escaped
strings back into them; instead compute an escaped local variable and use that
for rendering. For the property case, replace "property_def.text =
escape_tags(property_def.text)" with something like "escaped_text =
escape_tags(property_def.text)" and use escaped_text wherever the
template/render needs the escaped value; apply the same pattern for the other
places that reassign (the m.description occurrences) so you never overwrite
property_def.text or m.description in-place.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
doc/tools/make_md.py (2)
1516-1525:None-to-""fallback inescape_tags/escape_bracesis a latent footgun.Both functions accept
Optional[str]but returnstr, silently convertingNoneto"". All current callers guard againstNonebefore calling, so the internalNonebranch is never executed. If a future call site drops the guard (or calls the function directly on anOptionalfield), theNone-to-""conversion would silently transform a "no description" sentinel into an empty string, changing downstreamis not Nonechecks.Prefer making the type contract explicit:
♻️ Proposed fix
-def escape_tags(input: Optional[str]) -> str: - if input is None: - return "" - return input.replace("<", "<").replace(">", ">") +def escape_tags(input: str) -> str: + return input.replace("<", "<").replace(">", ">") -def escape_braces(input: Optional[str]) -> str: - if input is None: - return "" - return input.replace("{", "\\{").replace("}", "\\}") +def escape_braces(input: str) -> str: + return input.replace("{", "\\{").replace("}", "\\}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/tools/make_md.py` around lines 1516 - 1525, The functions escape_tags and escape_braces currently accept Optional[str] but silently convert None to "", which can mask "no description" sentinels; change their type contract to require a plain str (remove Optional from the parameter type) and delete the None fallback branch so they no longer silently coerce None. Update all call sites that previously passed Optional values to explicitly guard/handle None before calling (or propagate None as appropriate) so callers preserve the original None-vs-empty-string semantics.
2484-2486:HTML_TAG_REonly matches tags with at most one attribute — multi-attribute tags will have their</>incorrectly escaped.The attribute sub-pattern
(?:\s+[a-zA-Z0-9_/-]+(?:=...)?)?uses a single?, so only zero or one attribute is accepted. A tag like<a href="..." id="...">would not match, and its</>would be escaped by the even-index branch.For the current content (qualifier
<span class="...">,<hr class="...">,<div class="...">) one attribute is always sufficient. This becomes fragile if generated HTML ever uses multiple attributes.♻️ Proposed more robust regex (repeating attribute group)
-HTML_TAG_RE = re.compile( - r"(<(?:[a-zA-Z0-9_/]+(?:\s+[a-zA-Z0-9_/-]+(?:=\"[^\"]*\"|='[^']*'|[^>]*))?|!--.*?--|https?://.*?)>)" -) +HTML_TAG_RE = re.compile( + r"(<(?:[a-zA-Z0-9_/]+(?:\s+[a-zA-Z0-9_:/-]+(?:=\"[^\"]*\"|='[^']*')?)*|!--.*?--|https?://[^>]*)>)" +)Additionally, the double-check on line 2533 (
if not parts[i].startswith("<") or not parts[i].endswith(">")) is dead code: the regex's capturing group always starts with<and ends with>, so this branch can never execute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/tools/make_md.py` around lines 2484 - 2486, The HTML_TAG_RE currently allows at most one attribute, so change its attribute sub-pattern in HTML_TAG_RE to allow repeating attributes (make the attribute group quantified to * instead of ? so tags like <a href="..." id="..."> match correctly) and keep the other alternatives (comments/URLs) intact; also remove the dead check that verifies parts[i].startswith("<") or parts[i].endswith(">") because the regex capturing group always yields strings beginning with '<' and ending with '>' (delete that conditional branch where it appears).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/tools/make_md.py`:
- Around line 1245-1246: The pre-escaping call escape_tags on member description
fields (e.g., the assignment to m.description before format_text_block) corrupts
code-block content; remove the escape_tags pre-processing assignments entirely
(the one for m.description and the four analogous calls for
annotations/properties/constructors/methods/operators) so descriptions flow
unchanged into format_text_block which uses escape_md/HTML_TAG_RE and respects
inside_code; ensure no other code-paths call escape_tags on the full description
before format_text_block.
- Around line 1446-1447: The operator-detail output currently calls escape_tags
on the signature (variable signature produced by make_method_signature) which is
always a str and turns qualifier badge HTML (badge = '<span
class="const">const</span>') into escaped text; remove the dead None-guard and
stop calling escape_tags for the operator signature so the signature is written
raw like the constructor/method sections (i.e., write f.write(f"{ret_type}
{signature} {self_link}\n\n") using the unescaped signature), ensuring
escape_tags is still used where appropriate for other fields but not for the
operator signature.
- Around line 782-795: The hardcoded magic number position: 6 and the literal
label "Class Reference" should be made configurable and localizable: add a new
CLI option on args (e.g., args.class_ref_position or args.position_class_ref) or
declare a named constant near the top of the module for
DEFAULT_CLASS_REF_POSITION and use that instead of 6 when constructing
category_data, and replace the literal label with a call to translate("Class
Reference") to match other headings; ensure the new CLI arg is parsed and
documented so callers can override the position without editing source, and
update the code paths that reference
args.dry_run/category_json_path/category_data to use the new param/constant and
translate() for the label.
- Line 1735: The returned HTML string currently emits a non-self-closing hr tag
which breaks MDX/JSX parsing; update the return in make_md.py (the line that
builds the separator using separator_class) to use a self-closing hr void
element with the same class name and preserve the trailing newlines so the
output becomes a valid MDX-compatible JSX void element instead of "<hr
...></hr>".
---
Nitpick comments:
In `@doc/tools/make_md.py`:
- Around line 1516-1525: The functions escape_tags and escape_braces currently
accept Optional[str] but silently convert None to "", which can mask "no
description" sentinels; change their type contract to require a plain str
(remove Optional from the parameter type) and delete the None fallback branch so
they no longer silently coerce None. Update all call sites that previously
passed Optional values to explicitly guard/handle None before calling (or
propagate None as appropriate) so callers preserve the original
None-vs-empty-string semantics.
- Around line 2484-2486: The HTML_TAG_RE currently allows at most one attribute,
so change its attribute sub-pattern in HTML_TAG_RE to allow repeating attributes
(make the attribute group quantified to * instead of ? so tags like <a
href="..." id="..."> match correctly) and keep the other alternatives
(comments/URLs) intact; also remove the dead check that verifies
parts[i].startswith("<") or parts[i].endswith(">") because the regex capturing
group always yields strings beginning with '<' and ending with '>' (delete that
conditional branch where it appears).
- Updated anchor creation for consistency. - Added `_category_.json` generation for better doc indexing. - Introduced escaping functionality for special characters in descriptions and MD files. - Enhanced sanitization logic for class/file/operator names. - Streamlined Markdown reference links and tag escaping logic.
Arctis-Fireblight
force-pushed
the
Fix-Markdown-Script
branch
from
0b97c0e to
2b68ecb
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@doc/tools/make_md.py`:
- Around line 1445-1447: The operator signatures are being passed through
escape_tags which turns the HTML qualifier badges (e.g., <span
class="const">const</span>) into literal text; update the operator signature
emission to match constructors/methods by removing the escape_tags call and
write the signature raw (i.e., use the returned signature from
make_method_signature directly in the f.write call), and also remove the
unnecessary "if signature is not None else None" guard since
make_method_signature always returns a str; touch the code paths that call
escape_tags for operators (look for the lines using make_method_signature,
escape_tags, and the variable signature in this module) to make these changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_category_.jsongeneration for better doc indexing.Summary by CodeRabbit