Topic properties auto #126
Conversation
add treesitter working version working version performance
✅ Deploy Preview for docs-extensions-and-macros ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds a new automation CLI command topic-property-docs in bin/doc-tools.js to build topic property docs for a tag and optionally diff them against an old tag. Extends the Makefile with a topic-properties target that runs a new Python extractor (topic_property_extractor.py) to generate JSON and AsciiDoc outputs, plus updates .PHONY and adds a top-level build target. Updates generate_docs.py to produce a topic-properties.adoc page and classify topic-related definitions via DEFINED_IN_MAPPING. Introduces topic_property_extractor.py that discovers topic properties, enums, and cluster mappings, emits JSON, and can generate AsciiDoc. Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant CLI as bin/doc-tools.js (topic-property-docs)
participant MK as Makefile (topic-properties)
participant EX as topic_property_extractor.py
participant GD as generate_docs.py
participant FS as FS (autogenerated/)
Dev->>CLI: Run topic-property-docs --tag <newTag> [--diff <oldTag>]
alt --diff provided
CLI->>FS: Check autogenerated/<oldTag>/properties/ exists?
opt Missing
CLI->>MK: make topic-properties TAG=<oldTag>
MK->>EX: Run extractor -> topic-properties-output.json
MK->>EX: Generate topic-properties.adoc
MK->>GD: (existing flows for other pages)
EX->>FS: Write JSON/ADOC under autogenerated/<oldTag>/properties/
end
end
CLI->>MK: make topic-properties TAG=<newTag>
MK->>EX: Run extractor -> topic-properties-output.json
MK->>EX: Generate topic-properties.adoc
MK->>GD: (existing broker/cluster/cloud generation unaffected)
EX->>FS: Write JSON/ADOC under autogenerated/<newTag>/properties/
alt --diff provided
CLI->>FS: diffDirs('properties', oldTag, newTag)
FS-->>CLI: Patch under autogenerated/diffs/
end
CLI-->>Dev: Exit 0
note over EX,GD: New topic page: topic-properties.adoc<br/>Defined-in mapping includes topic sources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (9)
tools/property-extractor/topic_property_extractor.py (7)
7-7: Remove unused imports.The
TupleandSettypes are imported but never used in the code.Apply this diff to remove the unused imports:
-from typing import Dict, List, Optional, Tuple, Set +from typing import Dict, List, Optional
106-108: Silently catching all exceptions masks potential issues.The broad exception handling without any logging makes debugging difficult if file parsing fails for unexpected reasons.
Consider at least logging the error for debugging:
except Exception as e: # Skip files that can't be read - pass + print(f"Debug: Skipping {file_path}: {e}", file=sys.stderr)
317-317: Remove unused variable assignment.The variable
topic_candidatesis assigned but never used.Remove the unused variable assignment:
- topic_candidates = [] - # Remove common prefixes/suffixes cleaned = cluster_prop
182-184: Silent exception handling masks potential errors.Similar to the previous comment, catching all exceptions without logging makes debugging difficult.
Consider logging the error for debugging purposes:
except Exception as e: # Skip files that can't be read - pass + print(f"Debug: Error scanning enums in {file_path}: {e}", file=sys.stderr)
280-280: Unused exception variable.The exception variable
eis captured but never used.Apply this diff to clean up the unused variable:
- except Exception as e: + except Exception: return {}
476-477: Unnecessary directory existence check.The
os.makedirswithexist_ok=Truealready handles the case where the directory exists, making the conditional check redundant.Simplify the code by removing the unnecessary check:
- output_dir = os.path.dirname(output_path) - if output_dir: # Only create directory if there's a path - os.makedirs(output_dir, exist_ok=True) + os.makedirs(os.path.dirname(output_path), exist_ok=True)
412-481: Consider refactoring long method for better maintainability.The
generate_topic_properties_adocmethod is quite long (~70 lines) with hardcoded AsciiDoc content. Consider extracting the template content to a separate file or using a template engine for better maintainability.Would you like me to help refactor this method to use a template-based approach similar to how other documentation generators in this codebase work?
tools/property-extractor/Makefile (2)
1-1: Consider adding standard Makefile targets.The static analyzer correctly identifies that standard phony targets
allandtestare missing. While not critical, adding these would improve the Makefile's adherence to conventions.Add standard targets for better convention compliance:
-.PHONY: build venv clean redpanda-git treesitter topic-properties generate-docs check +.PHONY: all test build venv clean redpanda-git treesitter topic-properties generate-docs check + +# Default target +all: build + +# Test target (placeholder - add actual tests as needed) +test: + @echo "No tests configured yet"
114-115: Inconsistent output directory usage.The
topic-propertiestarget writes directly to$(OUTPUT_DIR)while the mainbuildtarget writes togen/first and then copies to$(OUTPUT_DIR). This inconsistency could cause confusion.Consider following the same pattern as the main build for consistency:
@cd $(TOOL_ROOT) && \ $(PYTHON) topic_property_extractor.py \ --source-path $(REDPANDA_SRC) \ - --output-json "$(OUTPUT_DIR)/topic-properties-output.json" \ - --output-adoc "$(OUTPUT_DIR)/topic-properties.adoc" + --output-json "gen/topic-properties-output.json" \ + --output-adoc "gen/topic-properties.adoc" + @echo "📄 Copying topic properties outputs to $(OUTPUT_DIR)…" + @mkdir -p "$(OUTPUT_DIR)" + @cp "$(TOOL_ROOT)/gen/topic-properties-output.json" "$(OUTPUT_DIR)/" + @cp "$(TOOL_ROOT)/gen/topic-properties.adoc" "$(OUTPUT_DIR)/" @echo "✅ Topic properties extracted"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
bin/doc-tools.js(1 hunks)tools/property-extractor/Makefile(2 hunks)tools/property-extractor/json-to-asciidoc/generate_docs.py(7 hunks)tools/property-extractor/topic_property_extractor.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/property-extractor/topic_property_extractor.py (1)
tools/property-extractor/json-to-asciidoc/generate_docs.py (1)
main(341-488)
🪛 Ruff (0.12.2)
tools/property-extractor/topic_property_extractor.py
7-7: typing.Tuple imported but unused
Remove unused import
(F401)
7-7: typing.Set imported but unused
Remove unused import
(F401)
106-106: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
182-182: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
280-280: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
317-317: Local variable topic_candidates is assigned to but never used
Remove assignment to unused variable topic_candidates
(F841)
🪛 checkmake (0.2.2)
tools/property-extractor/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 4-4: Target body for "build" exceeds allowed length of 5 (9).
(maxbodylength)
⏰ 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). (3)
- GitHub Check: Redirect rules - docs-extensions-and-macros
- GitHub Check: Header rules - docs-extensions-and-macros
- GitHub Check: Pages changed - docs-extensions-and-macros
🔇 Additional comments (4)
tools/property-extractor/json-to-asciidoc/generate_docs.py (3)
110-111: LGTM! Consistent with existing architecture pattern.The mapping addition for topic properties follows the established pattern of the existing configuration groups.
70-83: LGTM! Well-structured constants for topic properties page.The new constants follow the existing pattern and provide appropriate documentation for topic properties. The structure aligns well with the existing page documentation patterns for broker, cluster, and cloud properties.
358-358: LGTM! Topic properties cleanly integrated into the generation pipeline.The implementation follows the established pattern exactly, maintaining consistency with how broker, cluster, and cloud properties are processed and written.
Also applies to: 408-408, 444-449, 463-463
bin/doc-tools.js (1)
780-813: LGTM! Clean implementation following existing patterns.The new
topic-property-docscommand follows the established pattern of the existingproperty-docscommand perfectly, including proper error handling and diff functionality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Introduce new topic properties automation.
Extension of the existing cluster properties automation.
As opposed to other automations, the one about topic properties show significant challenges regarding min-max identification. Topic properties don't have internal descriptions. The existing ones on docs are hand-written.
This automation serves the purpose of identifying the new properties. However, a lot of manual work must be invested to make it "publish ready".
You can run it through makefile with:
or through the CLI with