feat(pre-commit): add internal-import alias check with auto-fix#49
feat(pre-commit): add internal-import alias check with auto-fix#49dengqiaoyu wants to merge 1 commit into
Conversation
- Add an AST-based hook enforcing code style guide §3.3: public modules must `_`-alias symbols imported from private modules; private modules must not - Auto-fix offending import statements and rename in-file references, refusing the fix only when the bound name is shadowed - Register the hook in `.pre-commit-config.yaml` and document it under §3.3 "Enforcement" - Apply across `src/coreai_opt/`, normalizing 27 modules with missing or redundant `_` aliases
dengqiaoyu
added
documentation
| yield from ast.walk(stmt) | ||
|
|
||
|
|
||
| def _locally_binds_name(node: ast.AST, name: str) -> bool: |
There was a problem hiding this comment.
2 comments on this function:
- It looks like it misses out on the case where t in
node.targetsitself is a tuple/list and when node is a comprehension. Updating the checks to use_is_named_targetcould dedupe some logic and fix these issues at the same time:
def _locally_binds_name(node: ast.AST, name: str) -> bool:
if isinstance(node, ast.Assign):
return any(_is_named_target(t, name) for t in node.targets)
if isinstance(node, (ast.AnnAssign, ast.AugAssign, ast.NamedExpr, ast.For,
ast.AsyncFor)):
return _is_named_target(node.target, name)
if isinstance(node, (ast.With, ast.AsyncWith)):
return any(
item.optional_vars is not None and
_is_named_target(item.optional_vars, name)
for item in node.items
)
if isinstance(node, ast.comprehension):
return _is_named_target(node.target, name)
return False
- This function and
_binds_assignment_targetshare very similar logic, could they be combined into one?
|
|
||
| def _node_span(node: _Positioned, line_starts: list[int]) -> tuple[int, int]: | ||
| """Convert an AST node's (lineno, col_offset, end_lineno, end_col_offset) to byte offsets.""" | ||
| start = line_starts[node.lineno - 1] + node.col_offset |
There was a problem hiding this comment.
Somewhat a corner case, but if the code has some character that is non-ASCII that uses more than one byte, the logic here can result in a malformed update:
# Before the fix
from temp_pkg._priv import helper
def use():
café = ...
return café + helper()
# After the fix
from temp_pkg._priv import helper as _helper
def use():
café = ...
return café + h_helper)
| """ | ||
| violations: list[Violation] = [] | ||
|
|
||
| for node in ast.walk(tree): |
There was a problem hiding this comment.
- Should we also check for and flag plain imports of private packages? Like
import pkg._private - Looks like we are missing checks for relative imports (
from ._priv import x)
| """ | ||
| original = alias.name | ||
| current_alias = alias.asname | ||
| if original.startswith("_") or current_alias not in (None, original, "_" + original): |
There was a problem hiding this comment.
Another corner case, this currently does not handle the case if someone names things with a custom alias (from a._b import c as d)
| ) | ||
|
|
||
|
|
||
| def _statement_edits( |
There was a problem hiding this comment.
If an import line has a comment at the end, looks like the comment would be removed as part of the rename
| symbols imported from private modules with a `_` prefix; private | ||
| modules must not. Auto-fixes the import statement and renames | ||
| references in the same file. | ||
| entry: python scripts/pre_commit/check_internal_import_aliases.py --fix |
There was a problem hiding this comment.
In general it seems like there are a lot of potential corner cases associated with automatically fixing the code. What do you think about making --fix opt-in instead where the user manually runs it if they want to have things be automatically fixed?
We could have the pre-commit hook only do the detection part of it to fail the commit if it finds any transgressions. I'm thinking that if any automatic fixes are made which break the code, other pre-commit hooks may also have run (linting, line too long, etc.) which would conflate the changes if the user needs to revert the code back to before this hook took action.
2 participants
_-alias symbols imported from private modules; private modules must not.pre-commit-config.yamland document it under §3.3 "Enforcement"src/coreai_opt/, normalizing 27 modules with missing or redundant_aliases