-
Notifications
You must be signed in to change notification settings - Fork 17
Add types, remove dead code, and beef up lints #76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the codebase to introduce stronger type annotations, remove dead code, and update the linting configuration. Key changes include updating the pyproject.toml settings (lint rules, dependencies, and python version constraints), refactoring shell command and git operations with enhanced type hints and logging, and modifying tests to accommodate the new git configuration structure.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pyproject.toml | Updated lint configuration, dependency definitions, and build setup |
src/stack_pr/shell_commands.py | Added type annotations, replaced legacy globals with logger usage |
src/stack_pr/git.py | Refactored git utilities, introduced GitConfig, and improved error handling |
tests/test_misc.py | Updated tests to reflect changes in git configuration and type usage |
src/stack_pr/init.py | Added module initialization stub |
Files not reviewed (1)
- src/stack_pr/py.typed: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to split NFC changes from this PR (in the spirit of stacked PR and for easier reviewing:) )?
return self._base | ||
|
||
@base.setter | ||
def base(self, base: str): | ||
def base(self, base: str | None) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe str | None
is a 3.10 feature, while we try to support 3.9, so I'd recommend the older syntax Optional[str]
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
github code review is wrong :) if you do from __future__ import annotations
you can do the modern syntax all the way back to python 3.7+.
Co-authored-by: Mikhail Zolotukhin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This is part of a larger cleanup refactor to strongly type much more of stack-pr.
Removes some dead code.
Move to some better patterns to make the new lint checks happy.