format and minor bug fixes#38
Conversation
WalkthroughRefactors deletion file operations with internal helpers and stricter None-path handling; adds backward-compatibility aliases in UI provider; updates imports and many tests to match internal helper usage and small formatting changes across scripts and tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/io/deletion.py (1)
102-106: Guard batch delete against img.path=None to avoid AttributeError.Line 103 still assumes
img.pathis always set, but single-delete now treats it as optional; batch delete can crash onNone. Consider mirroring the same guard.🛠️ Proposed fix
- for img in images: - file_names.append(img.path.name) - total_files += 1 - if img.raw_pair and img.raw_pair.exists(): - total_files += 1 + for img in images: + if img.path: + file_names.append(img.path.name) + total_files += 1 + else: + log.warning("confirm_batch_permanent_delete called with image_file.path=None") + if img.raw_pair and img.raw_pair.exists(): + total_files += 1
🤖 Fix all issues with AI agents
In `@repro_crash.py`:
- Around line 38-40: The except block that currently does "except Exception as
e: print(f\"CRASHED: {e}\"); raise e" should re-raise the caught exception with
its original traceback by replacing "raise e" with a bare "raise" in the same
except block (the handler that binds the exception to "e"), leaving the
print/log statement intact; locate the except Exception as e block in
repro_crash.py and make this single-line change.
In `@tests/test_highlights_v2.py`:
- Around line 19-20: The test calls _apply_headroom_shoulder with an incorrect
keyword argument name; change the call to use max_overshoot=steepness (i.e.,
replace steepness=steepness) and rename the local test variable from steepness
to max_overshoot (or update the docstring/variable to match) so names align with
the function signature; also update the test docstring/comment to reference
max_overshoot instead of steepness to keep naming consistent.
In `@verify_fix_auto_levels.py`:
- Around line 45-48: Remove the unused variable t1 assigned by t1 = time.time()
in the block that updates the file mtime (the img_path.touch() call); either
delete the t1 assignment entirely or use it where intended, but do not leave an
unused local variable named t1 next to the img_path.touch() call.
- Line 62: Remove the unused variable assignment target_path =
Path(saved_path).resolve() in verify_fix_auto_levels.py; either delete that line
entirely or replace subsequent uses to reference Path(saved_path).resolve()
directly (if intended), ensuring no leftover references to target_path remain
and leaving saved_path as the sole variable if nothing else needs changing.
🧹 Nitpick comments (4)
tests/test_prefetch_concurrency.py (1)
27-41: Silence Ruff ARG001 inmock_decode_and_cache.Rename unused varargs to indicate intent and keep lint clean.
♻️ Proposed fix
- def mock_decode_and_cache(*args, **kwargs): + def mock_decode_and_cache(*_args, **_kwargs): time.sleep(0.0001) # fast sleep return Path("/mock/image_x.jpg"), 1debug_al.py (1)
17-17: Consider prefixing unused variables with underscore.Static analysis flags
blacks,whites, andp_lowas unused. Since this is a debug script and onlyp_highis printed, you could use_placeholders for clarity:_, _, _, p_high = editor.auto_levels(threshold_percent=0.1)This is optional for a debug utility file.
reproduce_bug.py (1)
37-38: Remove unused variablet1.The variable
t1is assigned but never used. While it serves as documentation that "T1" is the new timestamp, it can be removed to clean up the code.♻️ Proposed fix
# 2. Save Main (update mtime to T1) - t1 = time.time() img_path.touch() # Updates mtimefaststack/io/deletion.py (1)
31-31: Remove the exception object fromlog.exception()calls—let the automatic traceback capture handle it.
logging.exception()automatically includes the current exception traceback, so passingeis redundant. Instead, log contextual information (path/filename) in the message to improve readability without duplicating the exception details.♻️ Proposed cleanup
- log.exception("Failed to create recycle bin directory: %s", e) + log.exception("Failed to create recycle bin directory: %s", recycle_bin_dir)- log.exception("Failed to permanently delete %s: %s", jpg_path.name, e) + log.exception("Failed to permanently delete %s", jpg_path.name)- log.exception("Failed to permanently delete %s: %s", raw_path.name, e) + log.exception("Failed to permanently delete %s", raw_path.name)Also applies to: 166, 175
| except Exception as e: | ||
| print(f"CRASHED: {e}") | ||
| raise e |
There was a problem hiding this comment.
Use bare raise to preserve the original traceback.
raise e resets context and can hide the true origin.
Suggested fix
- raise e
+ raise🧰 Tools
🪛 Ruff (0.14.14)
40-40: Use raise without specifying exception name
Remove exception name
(TRY201)
🤖 Prompt for AI Agents
In `@repro_crash.py` around lines 38 - 40, The except block that currently does
"except Exception as e: print(f\"CRASHED: {e}\"); raise e" should re-raise the
caught exception with its original traceback by replacing "raise e" with a bare
"raise" in the same except block (the handler that binds the exception to "e"),
leaving the print/log statement intact; locate the except Exception as e block
in repro_crash.py and make this single-line change.
| # 2. Save Main (update mtime to T1) | ||
| t1 = time.time() | ||
| img_path.touch() # Updates mtime | ||
| img_path.touch() # Updates mtime | ||
|
|
There was a problem hiding this comment.
Remove unused t1.
Line 46 assigns t1 but never uses it.
Suggested fix
- t1 = time.time()
img_path.touch() # Updates mtime📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 2. Save Main (update mtime to T1) | |
| t1 = time.time() | |
| img_path.touch() # Updates mtime | |
| img_path.touch() # Updates mtime | |
| # 2. Save Main (update mtime to T1) | |
| img_path.touch() # Updates mtime |
🧰 Tools
🪛 Ruff (0.14.14)
46-46: Local variable t1 is assigned to but never used
Remove assignment to unused variable t1
(F841)
🤖 Prompt for AI Agents
In `@verify_fix_auto_levels.py` around lines 45 - 48, Remove the unused variable
t1 assigned by t1 = time.time() in the block that updates the file mtime (the
img_path.touch() call); either delete the t1 assignment entirely or use it where
intended, but do not leave an unused local variable named t1 next to the
img_path.touch() call.
|
|
||
| # FIX LOGIC: | ||
| new_index = -1 | ||
| target_path = Path(saved_path).resolve() |
There was a problem hiding this comment.
Remove unused target_path.
It’s never referenced after assignment.
Suggested fix
- target_path = Path(saved_path).resolve()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| target_path = Path(saved_path).resolve() |
🧰 Tools
🪛 Ruff (0.14.14)
62-62: Local variable target_path is assigned to but never used
Remove assignment to unused variable target_path
(F841)
🤖 Prompt for AI Agents
In `@verify_fix_auto_levels.py` at line 62, Remove the unused variable assignment
target_path = Path(saved_path).resolve() in verify_fix_auto_levels.py; either
delete that line entirely or replace subsequent uses to reference
Path(saved_path).resolve() directly (if intended), ensuring no leftover
references to target_path remain and leaving saved_path as the sole variable if
nothing else needs changing.
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Style
Tests
✏️ Tip: You can customize this high-level summary in your review settings.