Update ruff linters #215
Merged
Update ruff linters #215
Conversation
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
Reviewer's GuideThis PR enables the full suite of Ruff lint rules by updating configurations and refactors core and example code to satisfy the new lint requirements—extracting utility functions, adding type annotations and signatures, removing redundant code, updating tests, and switching CI hooks. Class diagram for validate_stage_name extraction and StagedScript changesclassDiagram
class StagedScript {
+__init__(stages: list[str], console_force_terminal: Optional[bool] = None, console_log_path: bool = True, print_commands: bool = True) -> None
+parse_args(argv: list[str]) -> None
+raise_parser_error(message: str) -> NoReturn
+run(*, pretty_print: bool = False, print_command: Optional[bool] = None, **kwargs: Any) -> CompletedProcess
-args: Namespace
-commands_executed: list[str]
-console
-stages_to_run: set[str]
-start_time
}
class validate_stage_name {
+validate_stage_name(stage_name: str) -> None
}
StagedScript ..> validate_stage_name : uses
class RetryStage {
<<inherits>> TryAgain
}
class HelpFormatter {
<<inherits>> ArgumentDefaultsHelpFormatter
<<inherits>> RawDescriptionHelpFormatter
}
Class diagram for decorator and wrapper function changesclassDiagram
class stage {
+stage(stage_name: str, heading: str) -> Callable
}
class decorator {
+decorator(func: Callable) -> Callable
}
class wrapper {
+wrapper(self, *args: Any, **kwargs: Any) -> None
}
stage ..> validate_stage_name : uses
decorator o-- wrapper : returns
stage o-- decorator : returns
decorator ..> get_phase_method : uses
decorator ..> run_retryable_phases : uses
class get_phase_method {
+get_phase_method(self, method_name: str) -> Callable
}
class run_retryable_phases {
+run_retryable_phases(self, *args: Any, **kwargs: Any) -> None
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jmgate - I've reviewed your changes - here's some feedback:
- I noticed
validate_stage_namewas pulled out as a module‐level function but tests still callStagedScript.validate_stage_name—either reattach it to the class or update the tests to import and call it directly. - There are quite a few
# noqa: ANN*suppressions—consider refining those function signatures (e.g. naming and typing of*args/**kwargs) to satisfy the annotation rules instead of disabling them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- I noticed `validate_stage_name` was pulled out as a module‐level function but tests still call `StagedScript.validate_stage_name`—either reattach it to the class or update the tests to import and call it directly.
- There are quite a few `# noqa: ANN*` suppressions—consider refining those function signatures (e.g. naming and typing of `*args`/`**kwargs`) to satisfy the annotation rules instead of disabling them.
## Individual Comments
### Comment 1
<location> `staged_script/staged_script.py:61` </location>
<code_context>
+ Raises:
+ ValueError: If the stage name is invalid.
+ """
+ if not re.match("^[a-z]+$", stage_name):
+ message = (
+ f"Stage name {stage_name!r} must contain only lowercase letters."
</code_context>
<issue_to_address>
Using re.match for a simple lowercase check may be overkill.
Consider replacing the regex with: if not (stage_name.islower() and stage_name.isalpha()): ... for better readability and efficiency.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if not re.match("^[a-z]+$", stage_name):
message = (
f"Stage name {stage_name!r} must contain only lowercase letters."
)
raise ValueError(message)
=======
if not (stage_name.islower() and stage_name.isalpha()):
message = (
f"Stage name {stage_name!r} must contain only lowercase letters."
)
raise ValueError(message)
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `staged_script/staged_script.py:313` </location>
<code_context>
@functools.wraps(func)
- def wrapper(self, *args, **kwargs) -> None:
+ def wrapper(self, *args: Any, **kwargs: dict[str, Any]) -> None: # noqa: ANN001, ANN401
"""
Turn a function into a stage.
</code_context>
<issue_to_address>
Type annotation for **kwargs as dict[str, Any] may not be accurate.
PEP 484 suggests annotating **kwargs as Any rather than dict[str, Any], since **kwargs is not strictly a dict. Please update the annotation accordingly.
Suggested implementation:
```python
**kwargs: Any,
```
```python
def wrapper(self, *args: Any, **kwargs: Any) -> None: # noqa: ANN001, ANN401
```
</issue_to_address>
### Comment 3
<location> `test/test_staged_script.py:42` </location>
<code_context>
def test__validate_stage_name() -> None:
"""Test the :func:`validate_stage_name` method."""
- StagedScript._validate_stage_name("valid")
</code_context>
<issue_to_address>
Test for validate_stage_name should be updated to call the new standalone function.
Update the test to import and call validate_stage_name directly, rather than using StagedScript._validate_stage_name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
I should have done this ages ago. Instead of `_validate_stage_name` being a static method in the `StagedScript` class, make it a function in the `staged_script` module. We then no longer need to ignore the MyPy error about `__class__` being undefined, and Ruff's SLF linter won't complain about private member access.
jmgate
force-pushed
the
update-ruff-linters
branch
from
5a0d035 to
a2d6bb8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #215 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 180 176 -4
Branches 15 15
=========================================
- Hits 180 176 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jmgate
force-pushed
the
update-ruff-linters
branch
from
a2d6bb8 to
7793809
Compare
Collaborator
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @jmgate - I've reviewed your changes - here's some feedback:
- Remove any unused imports (e.g. the
remodule) after refactoringvalidate_stage_nameto avoid redundant code and lint warnings. - Instead of sprinkling
# noqa: ANN401in wrappers, consider fully annotating those parameters or configuring those exceptions in Ruff so you can drop the inline pragmas. - The new per-file ignores in
pyproject.tomlare quite broad; try scoping them more narrowly to only the specific rules or files that truly need them to avoid masking other issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove any unused imports (e.g. the `re` module) after refactoring `validate_stage_name` to avoid redundant code and lint warnings.
- Instead of sprinkling `# noqa: ANN401` in wrappers, consider fully annotating those parameters or configuring those exceptions in Ruff so you can drop the inline pragmas.
- The new per-file ignores in `pyproject.toml` are quite broad; try scoping them more narrowly to only the specific rules or files that truly need them to avoid masking other issues.
## Individual Comments
### Comment 1
<location> `staged_script/staged_script.py:45` </location>
<code_context>
rich.traceback.install()
+def validate_stage_name(stage_name: str) -> None:
+ """
+ Validate a stage name.
</code_context>
<issue_to_address>
The new validate_stage_name function uses islower() and isalpha(), which is not strictly equivalent to the previous regex.
The previous regex only allowed ASCII lowercase letters, while the new approach permits all Unicode lowercase letters. If you want to restrict to ASCII, add stage_name.isascii() or revert to the regex. If broader Unicode support is intended, this change is fine, but note the behavioral difference.
Suggested implementation:
```python
def validate_stage_name(stage_name: str) -> None:
"""
Validate a stage name.
Ensure a stage name consists of only ASCII lowercase letters. This is
both to simplify implementation details within the
:class:`StagedScript` class, and to provide the best user experience
for users of your :class:`StagedScript` subclasses.
Args:
stage_name: The name of the stage.
Raises:
```
```python
def validate_stage_name(stage_name: str) -> None:
"""
Validate a stage name.
Ensure a stage name consists of only ASCII lowercase letters. This is
both to simplify implementation details within the
:class:`StagedScript` class, and to provide the best user experience
for users of your :class:`StagedScript` subclasses.
Args:
stage_name: The name of the stage.
Raises:
ValueError: If the stage name contains non-ASCII, non-lowercase, or non-letter characters.
"""
if not (stage_name.isascii() and stage_name.islower() and stage_name.isalpha()):
raise ValueError(
f"Invalid stage name '{stage_name}'. Stage names must consist of only ASCII lowercase letters."
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jmgate
force-pushed
the
update-ruff-linters
branch
from
7793809 to
ef76d16
Compare
Collaborator
Author
|
All of the CodeFactor issues were preexisting—I'm not sure why it's complaining about them for this PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Type: Task
Description
Turn on all the latest and greatest Ruff linters, and adjust the code as needed to make them pass.
Summary by Sourcery
Enable all new Ruff linter rules and adjust the codebase, examples, and tests to satisfy updated lint and type requirements.
Enhancements:
Build:
CI:
Tests: