chore: add missing type hints (15 files)#64
Open
mangodxd wants to merge 1 commit intobmad-code-org:mainfrom
Open
chore: add missing type hints (15 files)#64mangodxd wants to merge 1 commit intobmad-code-org:mainfrom
mangodxd wants to merge 1 commit intobmad-code-org:mainfrom
Conversation
WalkthroughType annotations are added to Python script entry points and helper functions across multiple template and sample directories. All Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 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.
🧹 Nitpick comments (6)
samples/bmad-excalidraw/scripts/validate_excalidraw.py (1)
34-34: Consider adding return type annotation.The parameter type annotation is good. You could complete the signature by adding the return type
-> dict(based on the_build_resultreturn at line 214).📝 Proposed enhancement
-def validate(file_path: str): +def validate(file_path: str) -> dict:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/bmad-excalidraw/scripts/validate_excalidraw.py` at line 34, The validate function currently has a parameter type but no return type; update the signature of validate(file_path: str) to include the return type -> dict to reflect that it returns the result built by _build_result; locate the validate function and the _build_result call to ensure the annotation matches the actual returned structure.samples/bmad-excalidraw/scripts/generate_excalidraw.py (5)
174-175: PEP 484 violation: Use explicitT | Nonefor optional parameters.Multiple parameters with
Nonedefaults should use explicit optional syntax:start_id: int=None,end_id: int=None, andlabel: str=Noneshould bestart_id: int | None = None,end_id: int | None = None, andlabel: str | None = None.♻️ Proposed fix for PEP 484 compliance
-def make_arrow(x1, y1, x2, y2, start_id: int=None, end_id: int=None, label: str=None, - style: str="arrow", start_shape=None, end_shape=None): +def make_arrow(x1, y1, x2, y2, start_id: int | None = None, end_id: int | None = None, label: str | None = None, + style: str="arrow", start_shape=None, end_shape=None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/bmad-excalidraw/scripts/generate_excalidraw.py` around lines 174 - 175, The function signature for make_arrow uses bare types with None defaults which violates PEP 484; update the annotations to use explicit optional union syntax for parameters with None defaults by changing start_id: int=None to start_id: int | None = None, end_id: int=None to end_id: int | None = None, and label: str=None to label: str | None = None (leave other params unchanged) so type checkers correctly recognize optional types.
266-266: Consider adding return type annotation.The function returns a position dictionary. Adding
-> dictwould complete the type signature (matchinglayout_linearandlayout_radialat lines 304 and 373).📝 Proposed enhancement
-def layout_grid(elements: list, direction: str="LR", start_x: int=100, start_y: int=100): +def layout_grid(elements: list, direction: str="LR", start_x: int=100, start_y: int=100) -> dict:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/bmad-excalidraw/scripts/generate_excalidraw.py` at line 266, The function layout_grid lacks an explicit return type annotation; add "-> dict" to its signature so the type signature matches layout_linear and layout_radial and clearly indicates it returns a position dictionary. Update the def layout_grid(elements: list, direction: str="LR", start_x: int=100, start_y: int=100) declaration to include the return type annotation, leaving the implementation unchanged.
145-145: PEP 484 violation: Use explicitT | Nonefor optional parameter.The parameter
container_id: int=Noneshould becontainer_id: int | None = Noneper PEP 484.♻️ Proposed fix for PEP 484 compliance
-def make_text(x, y, text: str, container_id: int=None, width: int=200, height: int=80, font_size: int=20): +def make_text(x, y, text: str, container_id: int | None = None, width: int=200, height: int=80, font_size: int=20):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/bmad-excalidraw/scripts/generate_excalidraw.py` at line 145, The function signature for make_text uses the legacy optional annotation container_id: int=None; update it to use the PEP 484 union-with-None form by changing the parameter annotation to container_id: int | None = None (so the function declaration in make_text reflects container_id as an explicit optional type); ensure any other optional parameters follow the same pattern if present.
71-71: Consider adding return type annotation.The function returns a string (generated ID). Adding
-> strwould complete the type signature.📝 Proposed enhancement
-def generate_id(length: int=8): +def generate_id(length: int=8) -> str:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/bmad-excalidraw/scripts/generate_excalidraw.py` at line 71, The function generate_id currently has a typed parameter but lacks a return type; update its signature to include the return type annotation (-> str) so it reads like generate_id(length: int = 8) -> str to clearly indicate the function returns a string ID and complete the type signature.
123-123: PEP 484 violation: Use explicitT | Nonefor optional parameters.Per PEP 484, parameters with
Nonedefaults should use explicit optional type syntax. The parameterswidth: int=Noneandheight: int=Noneshould bewidth: int | None = Noneandheight: int | None = None.♻️ Proposed fix for PEP 484 compliance
-def make_shape(elem_type, x, y, label: str, width: int=None, height: int=None, bg_color=None) -> tuple: +def make_shape(elem_type, x, y, label: str, width: int | None = None, height: int | None = None, bg_color=None) -> tuple:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@samples/bmad-excalidraw/scripts/generate_excalidraw.py` at line 123, The function signature for make_shape uses bare defaults for optional parameters width and height (width: int=None, height: int=None) which violates PEP 484; update the annotations to use explicit optional union types (width: int | None = None and height: int | None = None) in the make_shape definition so the typing accurately reflects that these parameters may be None; keep the parameter order and defaults unchanged and only adjust the type annotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@samples/bmad-excalidraw/scripts/generate_excalidraw.py`:
- Around line 174-175: The function signature for make_arrow uses bare types
with None defaults which violates PEP 484; update the annotations to use
explicit optional union syntax for parameters with None defaults by changing
start_id: int=None to start_id: int | None = None, end_id: int=None to end_id:
int | None = None, and label: str=None to label: str | None = None (leave other
params unchanged) so type checkers correctly recognize optional types.
- Line 266: The function layout_grid lacks an explicit return type annotation;
add "-> dict" to its signature so the type signature matches layout_linear and
layout_radial and clearly indicates it returns a position dictionary. Update the
def layout_grid(elements: list, direction: str="LR", start_x: int=100, start_y:
int=100) declaration to include the return type annotation, leaving the
implementation unchanged.
- Line 145: The function signature for make_text uses the legacy optional
annotation container_id: int=None; update it to use the PEP 484 union-with-None
form by changing the parameter annotation to container_id: int | None = None (so
the function declaration in make_text reflects container_id as an explicit
optional type); ensure any other optional parameters follow the same pattern if
present.
- Line 71: The function generate_id currently has a typed parameter but lacks a
return type; update its signature to include the return type annotation (-> str)
so it reads like generate_id(length: int = 8) -> str to clearly indicate the
function returns a string ID and complete the type signature.
- Line 123: The function signature for make_shape uses bare defaults for
optional parameters width and height (width: int=None, height: int=None) which
violates PEP 484; update the annotations to use explicit optional union types
(width: int | None = None and height: int | None = None) in the make_shape
definition so the typing accurately reflects that these parameters may be None;
keep the parameter order and defaults unchanged and only adjust the type
annotations.
In `@samples/bmad-excalidraw/scripts/validate_excalidraw.py`:
- Line 34: The validate function currently has a parameter type but no return
type; update the signature of validate(file_path: str) to include the return
type -> dict to reflect that it returns the result built by _build_result;
locate the validate function and the _build_result call to ensure the annotation
matches the actual returned structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b045e56f-9572-4167-a191-690d3ab90669
📒 Files selected for processing (15)
samples/bmad-agent-dream-weaver/scripts/merge-config.pysamples/bmad-agent-dream-weaver/scripts/merge-help-csv.pysamples/bmad-agent-dream-weaver/scripts/recall_metrics.pysamples/bmad-agent-dream-weaver/scripts/seed_tracker.pysamples/bmad-agent-dream-weaver/scripts/symbol_stats.pysamples/bmad-excalidraw/scripts/generate_excalidraw.pysamples/bmad-excalidraw/scripts/validate_excalidraw.pyskills/bmad-bmb-setup/scripts/cleanup-legacy.pyskills/bmad-bmb-setup/scripts/merge-config.pyskills/bmad-bmb-setup/scripts/merge-help-csv.pyskills/bmad-module-builder/assets/setup-skill-template/scripts/cleanup-legacy.pyskills/bmad-module-builder/assets/setup-skill-template/scripts/merge-config.pyskills/bmad-module-builder/assets/setup-skill-template/scripts/merge-help-csv.pyskills/bmad-module-builder/assets/standalone-module-template/merge-config.pyskills/bmad-module-builder/assets/standalone-module-template/merge-help-csv.py
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.
Hello,
I noticed a few areas where the code could be made a bit more robust and readable. This PR focuses on minor housekeeping tasks and does not change any of the existing runtime logic:
Type hint additions
cleanup-legacy.py: added type hints to 1 function(s)cleanup-legacy.py: added type hints to 1 function(s)generate_excalidraw.py: added type hints to 10 function(s)merge-config.py: added type hints to 1 function(s)merge-config.py: added type hints to 1 function(s)merge-config.py: added type hints to 1 function(s)merge-config.py: added type hints to 1 function(s)merge-help-csv.py: added type hints to 1 function(s)merge-help-csv.py: added type hints to 1 function(s)merge-help-csv.py: added type hints to 1 function(s)merge-help-csv.py: added type hints to 1 function(s)recall_metrics.py: added type hints to 1 function(s)seed_tracker.py: added type hints to 1 function(s)symbol_stats.py: added type hints to 1 function(s)validate_excalidraw.py: added type hints to 2 function(s)I have added basic type annotations based on variable names and default values. This should make the code easier to work with in modern IDEs without changing any of the actual logic.
Examples of changes
Example change in
cleanup-legacy.py:Example change in
cleanup-legacy.py:Safety and verification
ast.parse()to ensure no syntax errors were introduced.eval()orglobals()were automatically skipped.__init__.pyfiles.Let me know if you would like me to change anything here.
Summary by CodeRabbit