Skip to content
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.

Sign up for GitHub

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

Close file handles if close_on_exit even if safer.writer() fails; use future type annotations #28

Merged
merged 3 commits into from
Nov 5, 2024
Merged

Close file handles if close_on_exit even if safer.writer() fails; use future type annotations #28

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e7bd32
Close file handles if close_on_exit even if safer.writer() fails
rec Nov 2, 2024
9aef72b
Use future type annotations
rec Nov 2, 2024
f61c824
Get rid of string interpolation
rec Nov 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 79 additions & 71 deletions safer/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@
# Either the whole file is written, or nothing

"""
from __future__ import annotations

import contextlib
import functools
import io
Expand All @@ -162,15 +164,15 @@


def writer(
stream: t.Union[t.Callable, None, t.IO, Path, str] = None,
is_binary: t.Optional[bool] = None,
stream: t.Callable | None | t.IO | Path | str = None,
is_binary: bool | None = None,
close_on_exit: bool = False,
temp_file: bool = False,
chunk_size: int = 0x100000,
delete_failures: bool = True,
dry_run: t.Union[bool, t.Callable] = False,
dry_run: bool | t.Callable = False,
enabled: bool = True,
) -> t.Union[t.Callable, t.IO]:
) -> t.Callable | t.IO:
"""
Write safely to file streams, sockets and callables.

Expand Down Expand Up @@ -231,78 +233,84 @@ def writer(
if not enabled:
return stream

write: t.Optional[t.Callable]
write: t.Callable | None

if callable(dry_run):
write, dry_run = dry_run, True
if close_on_exit and stream in (sys.stdout, sys.stderr):
raise ValueError('You cannot close stdout or stderr')

elif dry_run:
write = len
if dry_run:
close_on_exit = False

elif close_on_exit and hasattr(stream, 'write'):
if temp_file and BUG_MESSAGE:
raise NotImplementedError(BUG_MESSAGE)
try:
if callable(dry_run):
write, dry_run = dry_run, True

def write(v):
with stream:
stream.write(v)
elif dry_run:
write = len

else:
write = getattr(stream, 'write', None)
elif close_on_exit and hasattr(stream, 'write'):
if temp_file and BUG_MESSAGE:
raise NotImplementedError(BUG_MESSAGE)

send = getattr(stream, 'send', None)
mode = getattr(stream, 'mode', None)
def write(v):
with stream:
stream.write(v)

if dry_run:
close_on_exit = False
else:
write = getattr(stream, 'write', None)

if close_on_exit and stream in (sys.stdout, sys.stderr):
raise ValueError('You cannot close stdout or stderr')
send = getattr(stream, 'send', None)
mode = getattr(stream, 'mode', None)

if write and mode:
if not set('w+a').intersection(mode):
raise ValueError('Stream mode "%s" is not a write mode' % mode)
if write and mode:
if not set('w+a').intersection(mode):
raise ValueError(f'Stream mode "{mode}" is not a write mode')

binary_mode = 'b' in mode
if is_binary is not None and is_binary is not binary_mode:
raise ValueError('is_binary is inconsistent with the file stream')
binary_mode = 'b' in mode
if is_binary is not None and is_binary is not binary_mode:
raise ValueError('is_binary is inconsistent with the file stream')

is_binary = binary_mode
is_binary = binary_mode

elif dry_run:
pass
elif dry_run:
pass

elif send and hasattr(stream, 'recv'): # It looks like a socket:
if not (is_binary is None or is_binary is True):
raise ValueError('is_binary=False is inconsistent with a socket')
elif send and hasattr(stream, 'recv'): # It looks like a socket:
if not (is_binary is None or is_binary is True):
raise ValueError('is_binary=False is inconsistent with a socket')

write = send
is_binary = True
write = send
is_binary = True

elif callable(stream):
write = stream
elif callable(stream):
write = stream

else:
raise ValueError('Stream is not a file, a socket, or callable')

closer: _StreamCloser

if temp_file:
closer = _FileStreamCloser(
write,
close_on_exit,
is_binary,
temp_file,
chunk_size,
delete_failures,
)
else:
closer = _MemoryStreamCloser(write, close_on_exit, is_binary)
else:
raise ValueError('Stream is not a file, a socket, or callable')

closer: _StreamCloser

if temp_file:
closer = _FileStreamCloser(
write,
close_on_exit,
is_binary,
temp_file,
chunk_size,
delete_failures,
)
else:
closer = _MemoryStreamCloser(write, close_on_exit, is_binary)

if send is write:
closer.fp.send = write

if send is write:
closer.fp.send = write
return closer.fp

return closer.fp
except Exception:
if close_on_exit:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception is reraised in all code paths!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

getattr(stream, 'close', lambda: None)()
raise


# There's an edge case in #23 I can't yet fix, so I fail
Expand All @@ -311,18 +319,18 @@ def write(v):


def open(
name: t.Union[Path, str],
name: Path | str,
mode: str = 'r',
buffering: int = -1,
encoding: t.Optional[str] = None,
errors: t.Optional[str] = None,
newline: t.Optional[str] = None,
encoding: str | None = None,
errors: str | None = None,
newline: str | None = None,
closefd: bool = True,
opener: t.Optional[t.Callable] = None,
opener: t.Callable | None = None,
make_parents: bool = False,
delete_failures: bool = True,
temp_file: bool = False,
dry_run: t.Union[bool, t.Callable] = False,
dry_run: bool | t.Callable = False,
enabled: bool = True,
) -> t.IO:
"""
Expand Down Expand Up @@ -385,7 +393,7 @@ def open(
name = str(name)

if not isinstance(name, str):
raise TypeError('`name` must be string, not %s' % type(name).__name__)
raise TypeError(f'`name` must be string, not {type(name).__name__}')

name = os.path.realpath(name)
parent = os.path.dirname(os.path.abspath(name))
Expand Down Expand Up @@ -431,7 +439,7 @@ def simple_write(value):
raise ValueError("binary mode doesn't take an errors argument")

if 'x' in mode and os.path.exists(name):
raise FileExistsError("File exists: '%s'" % name)
raise FileExistsError(f"File exists: '{name}'")

if buffering == -1:
buffering = io.DEFAULT_BUFFER_SIZE
Expand All @@ -447,8 +455,8 @@ def simple_write(value):


def closer(
stream: t.IO, is_binary: t.Optional[bool] = None, close_on_exit: bool = True, **kwds
) -> t.Union[t.Callable, t.IO]:
stream: t.IO, is_binary: bool | None = None, close_on_exit: bool = True, **kwds
) -> t.Callable | t.IO:
"""
Like `safer.writer()` but with `close_on_exit=True` by default

Expand All @@ -460,7 +468,7 @@ def closer(

def dump(
obj,
stream: t.Union[t.Callable, None, t.IO, Path, str] = None,
stream: t.Callable | None | t.IO | Path | str = None,
dump: t.Any = None,
**kwargs,
) -> t.Any:
Expand Down Expand Up @@ -533,8 +541,8 @@ def _get_dumper(dump: t.Any) -> t.Callable:

@contextlib.contextmanager
def printer(
name: t.Union[Path, str], mode: str = 'w', *args, **kwargs
) -> t.Generator[t.Callable, None, None]:
name: Path | str, mode: str = 'w', *args, **kwargs
) -> t.Iterator[t.Callable]:
"""
A context manager that yields a function that prints to the opened file,
only writing to the original file at the exit of the context,
Expand Down
Loading