-
Notifications
You must be signed in to change notification settings - Fork 0
Ruff processors #32
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
base: main
Are you sure you want to change the base?
Ruff processors #32
Conversation
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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 adds Ruff support to ruffen-docs by introducing two new processor classes (RuffChecker and RuffFormatter) alongside the existing BlackFormatter. The module is renamed from formatters to processors to better reflect its expanded purpose.
Key Changes:
- Added
RuffCheckerandRuffFormatterprocessor classes that use subprocess to call ruff commands - Renamed module from
formatterstoprocessorsand updated imports - Added two new pre-commit hooks:
ruffen-docs-checkandruffen-docs-format
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/ruffen_docs/processors.py |
Added subprocess import, ruff.main import, and two new processor classes (RuffChecker and RuffFormatter) |
src/ruffen_docs/__init__.py |
Updated import to use processors module instead of formatters |
tests/test_ruffen_docs.py |
Updated import to use processors module instead of formatters |
.pre-commit-config.yaml |
Added two new pre-commit hooks for ruffen-docs-check and ruffen-docs-format with ruff 0.14.10 dependency |
Comments suppressed due to low confidence (6)
src/ruffen_docs/processors.py:364
- The commented out check=True parameter should either be removed or uncommented with proper error handling. If check=True is uncommented, subprocess.run will raise CalledProcessError automatically on non-zero exit, making the subsequent check_returncode() call redundant. Currently, using check_returncode() separately is correct, but the commented code creates ambiguity about the intended approach.
src/ruffen_docs/processors.py:337 - The init methods in both RuffChecker and RuffFormatter are redundant as they only call super().init() without adding any initialization logic. These methods can be removed to simplify the code.
src/ruffen_docs/processors.py:355 - The init methods in both RuffChecker and RuffFormatter are redundant as they only call super().init() without adding any initialization logic. These methods can be removed to simplify the code.
src/ruffen_docs/processors.py:350 - The RuffChecker.process_code_block method returns result.stdout, but ruff check does not output the formatted code to stdout - it outputs diagnostic messages. The process_code_block method should return the original code_block unchanged since ruff check is meant to validate, not transform the code. This is a fundamental difference from ruff format which actually outputs the formatted code.
src/ruffen_docs/processors.py:368 - The newly added RuffChecker and RuffFormatter classes lack test coverage. Since the existing BlackFormatter has comprehensive test coverage (as seen in tests/test_ruffen_docs.py with numerous test functions), these new processor classes should also have corresponding tests to verify their behavior with various code block formats (markdown, RST, pycon, etc.).
src/ruffen_docs/processors.py:346 - The commented out check=True parameter should either be removed or uncommented with proper error handling. If check=True is uncommented, subprocess.run will raise CalledProcessError automatically on non-zero exit, making the subsequent check_returncode() call redundant. Currently, using check_returncode() separately is correct, but the commented code creates ambiguity about the intended approach.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
"formatter" is not the best term to cover ruff's "check" and "format" behaviour together
ed011b3 to
fbb9e4d
Compare
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
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (6)
src/ruffen_docs/processors.py:368
- The new RuffChecker and RuffFormatter classes lack test coverage. Similar tests should be added for these new processors, following the same pattern as the existing BlackFormatter tests in this file.
src/ruffen_docs/processors.py:348 - The subprocess call does not specify a timeout parameter, which could cause the process to hang indefinitely if ruff encounters an issue. Consider adding a timeout to prevent the process from blocking forever.
src/ruffen_docs/processors.py:349 - Calling check_returncode() will raise a CalledProcessError when ruff check finds linting violations (non-zero exit code), which will be caught by the _collect_error context manager and treated as a processing error. For a checker, this is incorrect behavior - finding linting violations should not be treated as a processing error. The method should handle the return code appropriately and return the original code_block unchanged.
src/ruffen_docs/processors.py:350 - The RuffChecker.process_code_block method returns result.stdout, but ruff check typically outputs diagnostics to stderr and returns an empty stdout for valid code. This will likely return an empty string even when the check passes, which may not be the intended behavior. Consider returning the original code_block when the check passes, or handle the output differently.
src/ruffen_docs/processors.py:366 - The subprocess call does not specify a timeout parameter, which could cause the process to hang indefinitely if ruff encounters an issue. Consider adding a timeout to prevent the process from blocking forever.
src/ruffen_docs/processors.py:364 - These commented-out lines should either be removed if they're not needed, or uncommented and implemented if they are. Leaving commented code in the codebase reduces clarity about what the intended behavior should be.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| retv = 0 | ||
| for filename in args.filenames: | ||
| print(filename) |
Copilot
AI
Jan 3, 2026
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.
This debug print statement should be removed. It appears to have been added during development and will output filenames to stdout during normal operation, which could interfere with the tool's output and usage in automated environments.
| print(filename) |
| filenames = [] | ||
|
|
||
| retv = 0 | ||
|
|
||
| for filename in filenames: |
Copilot
AI
Jan 3, 2026
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.
The filenames list is initialized as empty but is never populated with args.filenames. This means the loop will never execute, and no files will be processed. You should use args.filenames directly in the loop, similar to how run_check and run_black are implemented.
| filenames = [] | |
| retv = 0 | |
| for filename in filenames: | |
| retv = 0 | |
| for filename in args.filenames: |
| parser = argparse.ArgumentParser() | ||
| parser.add_argument( | ||
| "-l", | ||
| "--line-length", | ||
| type=int, | ||
| default=DEFAULT_LINE_LENGTH, | ||
| ) | ||
| parser.add_argument( | ||
| "-S", | ||
| "--skip-string-normalization", | ||
| action="store_true", | ||
| ) | ||
| parser.add_argument("-E", "--skip-errors", action="store_true") | ||
| parser.add_argument("filenames", nargs="*") | ||
| args = parser.parse_args(argv) |
Copilot
AI
Jan 3, 2026
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.
The run_check and run_format functions accept line-length and skip-string-normalization arguments that are parsed but never used. These parameters should either be passed to the RuffChecker and RuffFormatter constructors (if they support configuration), or the argument parsing should be removed to avoid misleading users.
| parser = argparse.ArgumentParser() | ||
| parser.add_argument( | ||
| "-l", | ||
| "--line-length", | ||
| type=int, | ||
| default=DEFAULT_LINE_LENGTH, | ||
| ) | ||
| parser.add_argument( | ||
| "-S", | ||
| "--skip-string-normalization", | ||
| action="store_true", | ||
| ) | ||
| parser.add_argument("-E", "--skip-errors", action="store_true") | ||
| parser.add_argument("filenames", nargs="*") | ||
| args = parser.parse_args(argv) | ||
|
|
||
| retv = 0 | ||
|
|
||
| for filename in args.filenames: | ||
| formatter = RuffChecker() | ||
| retv |= formatter.format_file( | ||
| filename, | ||
| skip_errors=args.skip_errors, | ||
| # rst_literal_blocks=args.rst_literal_blocks, | ||
| # check_only=args.check, | ||
| ) | ||
|
|
||
| return retv | ||
|
|
||
|
|
||
| def run_format(argv: Sequence[str] | None = None) -> int: | ||
| ... | ||
| return 1 | ||
| parser = argparse.ArgumentParser() | ||
| parser.add_argument( | ||
| "-l", | ||
| "--line-length", | ||
| type=int, | ||
| default=DEFAULT_LINE_LENGTH, | ||
| ) | ||
| parser.add_argument( | ||
| "-S", | ||
| "--skip-string-normalization", | ||
| action="store_true", | ||
| ) | ||
| parser.add_argument("-E", "--skip-errors", action="store_true") | ||
| parser.add_argument("filenames", nargs="*") | ||
| args = parser.parse_args(argv) |
Copilot
AI
Jan 3, 2026
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.
The argument parsing code in run_check and run_format is largely duplicated from run_black. Consider extracting this into a shared helper function to reduce code duplication and improve maintainability.
| # rst_literal_blocks=args.rst_literal_blocks, | ||
| # check_only=args.check, | ||
| ) |
Copilot
AI
Jan 3, 2026
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.
These commented-out lines should either be removed if they're not needed, or uncommented and implemented if they are. Leaving commented code in the codebase reduces clarity about what the intended behavior should be.
| formatter = RuffChecker() | ||
| retv |= formatter.format_file( |
Copilot
AI
Jan 3, 2026
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.
The variable name 'formatter' is misleading when the instance is actually a RuffChecker, not a formatter. Consider renaming to 'checker' for clarity and consistency with the class name.
| formatter = RuffChecker() | |
| retv |= formatter.format_file( | |
| checker = RuffChecker() | |
| retv |= checker.format_file( |
| # rst_literal_blocks=args.rst_literal_blocks, | ||
| # check_only=args.check, | ||
| ) |
Copilot
AI
Jan 3, 2026
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.
These commented-out lines should either be removed if they're not needed, or uncommented and implemented if they are. Leaving commented code in the codebase reduces clarity about what the intended behavior should be.
No description provided.