Close file handles if close_on_exit even if safer.writer() fails; use future type annotations#28
Conversation
Reviewer's Guide by SourceryThis PR makes two main improvements to the safer library: it ensures proper file handle cleanup when close_on_exit is True even if safer.writer() fails, and modernizes type annotations using Python's newer union operator syntax (|). The changes are implemented by wrapping the main writer logic in a try-except block and updating type hints throughout the codebase. Updated class diagram for the safer moduleclassDiagram
class Safer {
+writer(stream: Callable | None | IO | Path | str, is_binary: bool | None, close_on_exit: bool, temp_file: bool, chunk_size: int, delete_failures: bool, dry_run: bool | Callable, enabled: bool) Callabe | IO
+open(name: Path | str, mode: str, buffering: int, encoding: str | None, errors: str | None, newline: str | None, closefd: bool, opener: Callable | None, make_parents: bool, delete_failures: bool, temp_file: bool, dry_run: bool | Callable, enabled: bool) IO
+closer(stream: IO, is_binary: bool | None, close_on_exit: bool, **kwds) Callable | IO
+dump(obj, stream: Callable | None | IO | Path | str, dump: Any, **kwargs) Any
+printer(name: Path | str, mode: str, *args, **kwargs) Iterator[Callable]
}
note for Safer "Updated type annotations using Python's newer union operator syntax (|)"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @rec - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| return closer.fp | ||
| except Exception: | ||
| if close_on_exit: |
There was a problem hiding this comment.
issue (bug_risk): The error handler could mask the original exception if close() fails
Consider using contextlib.suppress() or logging any cleanup errors while preserving the original exception.
There was a problem hiding this comment.
The exception is reraised in all code paths!
There was a problem hiding this comment.
Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:
- Avoid comments that suggest changes without clear justification or evidence of a problem.
- Ensure comments identify specific issues with potential impact, such as bugs or performance concerns.
- Provide actionable suggestions that are directly related to the identified issue.
| raise ValueError('Stream mode "%s" is not a write mode' % mode) | ||
| if write and mode: | ||
| if not set('w+a').intersection(mode): | ||
| raise ValueError('Stream mode "%s" is not a write mode' % mode) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Replace interpolated string formatting with f-string (
replace-interpolation-with-fstring) - Simplify logical expression using De Morgan identities (
de-morgan)
There was a problem hiding this comment.
- The first comment is good and I got rid of all string interpolation.
- The second comment is not good, there is no way to simplify the condition.
There was a problem hiding this comment.
Thank you for your feedback, we will generate fewer comments like this in the future according to the following instructions:
- Avoid including comments that list multiple issues without specific examples or context from the code.
- Do not include comments that rely solely on external links for explanation without providing a summary or rationale within the comment itself.
- Ensure comments provide actionable feedback that is directly applicable to the code being reviewed.
|
@Chiemezuo: these are three tiny commits but I thought each would be educational for you. |
|
Thanks! |
Summary by Sourcery
Close file handles when 'close_on_exit' is set, even if 'safer.writer()' fails, and update type annotations to use future syntax for better readability.
Bug Fixes:
Enhancements: