Conversation
Signed-off-by: varun-r-mallya <varunrmallya@gmail.com>
|
please check for breakages @r41k0u on your side |
There was a problem hiding this comment.
Pull Request Overview
This PR adds static type checking support to the codebase by enabling mypy type checking and adding type annotations to improve code quality and maintainability.
- Enables mypy type checking in pre-commit configuration
- Adds type annotations to class attributes and function parameters/return types
- Excludes tests and examples from type checking and formatting
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pythonbpf/maps/maps_utils.py | Adds type annotations for the _processors class attribute |
| pythonbpf/functions_pass.py | Adds type annotations for global variable and function return type |
| pythonbpf/codegen.py | Wraps subprocess call result in bool() for type consistency |
| .pre-commit-config.yaml | Enables mypy type checking and updates exclusion patterns |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| from .expr_pass import eval_expr, handle_expr | ||
|
|
||
| local_var_metadata = {} | ||
| local_var_metadata: dict[str | Any, Any] = {} |
There was a problem hiding this comment.
The type annotation dict[str | Any, Any] is overly permissive. Consider using a more specific type for the key (e.g., just str if all keys are strings) or define a TypedDict if the structure is predictable.
| local_var_metadata: dict[str | Any, Any] = {} | |
| local_var_metadata: dict[str, str] = {} |
pythonbpf/functions_pass.py
Outdated
|
|
||
|
|
||
| def infer_return_type(func_node: ast.FunctionDef): | ||
| def infer_return_type(func_node: ast.FunctionDef) -> str | None | Any: |
There was a problem hiding this comment.
The return type str | None | Any defeats the purpose of type checking. Consider using a more specific union type or Optional[str] if the function can return a string or None.
| success = compile_to_ir(str(caller_file), str(ll_file)) and success | ||
|
|
||
| success = ( | ||
| success = bool( |
There was a problem hiding this comment.
The explicit bool() wrapper is unnecessary. The subprocess.run() result in a boolean context already evaluates correctly for the and operation on line 142.
Signed-off-by: varun-r-mallya <varunrmallya@gmail.com>
Adds static type checking to code. Does not check examples and tests.