Address review feedback on docstrings and the placeholder-cleanup except

- _move_file_to_archive's docstring no longer claims the copy2 fallback
  replaces the placeholder atomically; only the same-filesystem os.rename
  path is atomic. The fallback is a plain copy-and-overwrite, which is
  still collision-safe because the placeholder already claimed the name.
- The empty except OSError in the placeholder cleanup now carries a
  comment explaining that it is deliberate best-effort cleanup and the
  re-raised move failure is the actionable error.
- test_general_archive_directory_unset_leaves_attribute_absent's
  docstring now describes what the assertion actually tests (the
  attribute staying absent from a bare Namespace) and moves the
  real-CLI None-default behavior to a parenthetical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Sean Whalen
2026-07-25 22:42:31 -04:00
co-authored by Claude Fable 5
parent c9ab999c4e
commit 117ef9e827
2 changed files with 13 additions and 4 deletions
+8 -2
View File
@@ -314,8 +314,12 @@ def _move_file_to_archive(file_path: str, dest_dir: str) -> str:
existing destination on POSIX, which would violate the
never-overwrite guarantee. Instead, each candidate name is staked
out with a zero-byte placeholder file before the real move happens;
``os.rename`` (POSIX) or the ``copy2`` fallback (Windows) that
backs ``shutil.move()`` then replaces that placeholder atomically.
``shutil.move()`` then replaces that placeholder with the real file
— atomically via ``os.rename`` when source and destination are on
the same POSIX filesystem, otherwise (Windows, or a cross-device
move) via a ``copy2``-and-overwrite that is not atomic but still
cannot collide with a concurrent invocation, since the placeholder
already claimed the name.
"""
os.makedirs(dest_dir, exist_ok=True)
base, ext = os.path.splitext(os.path.basename(file_path))
@@ -338,6 +342,8 @@ def _move_file_to_archive(file_path: str, dest_dir: str) -> str:
try:
os.remove(dest_path)
except OSError:
# Best-effort cleanup of the just-created placeholder; the
# move failure re-raised below is the error that matters.
pass
raise
return dest_path
+5 -2
View File
@@ -4525,8 +4525,11 @@ class TestParseConfigGeneral(unittest.TestCase):
def test_general_archive_directory_unset_leaves_attribute_absent(self):
"""When archive_directory is absent from the INI, _parse_config
never sets opts.archive_directory (the CLI's Namespace default of
None applies instead)."""
never sets opts.archive_directory at all — asserted here as the
attribute staying absent from this test's bare Namespace. (In the
real CLI the attribute pre-exists with the Namespace default of
None, so _parse_config leaving it untouched is what keeps
archiving disabled.)"""
from parsedmarc.cli import _parse_config
cp = _config_with("general", {"silent": "false"})