Skip to content

Three follow-ups from #873: pin except BaseException, fix a false docstring clause, drop an inert pylint pragma #875

Closed

Description

@JarryShaw

Three follow-ups from #873's review that landed after 687721091 merged. None is a correctness defect in the merged code — one is an unpinned invariant and two are prose/lint.

1. except BaseException: in _snapshot_and_restore is unpinned. Mutating it to except Exception: leaves tests/vendor/test_vendor_snapshot_restore_unit.py fully green — measured, run=3 failures=0 errors=0. The consequence under that mutant, for a KeyboardInterrupt mid-crawl, is a truncated const file and an orphaned .const.py.*.bak inside pcapkit/const/, where the merged code restores cleanly and leaves ['const.py']. A test whose stub raises KeyboardInterrupt after truncating the destination closes it: assert the content is restored byte-for-byte, that no .bak survives, and that the KeyboardInterrupt still propagates rather than becoming a False return.

2. A false clause in _snapshot_and_restore's docstring (pcapkit/vendor/__main__.py:73-75): "this crawler goes on to fail loudly anyway once Vendor.__init__ calls the same _dest_path itself." True of the first trigger it enumerates (a crawler outside the real pcapkit.vendor tree); false of the second — an instance-method _dest_path raises TypeError on the class call so the snapshot is skipped, but resolves fine on the instance call, so the target runs unprotected and succeeds silently (run() → True, content replaced, no warning). tests/vendor/test_vendor_snapshot_restore_unit.py's own module docstring describes this correctly, so the two disagree.

3. The # pylint: disable=no-else-raise pragma is inert. R1720 applies to if/elif/else after a raise, never to try/except/else. With it removed and --enable=no-else-raise the file still scores 10.00/10, and pylint reports I0021: Useless suppression of 'no-else-raise'. Not a CI gate — PYLINT_FLAGS in the Makefile enables F,E,W,R and never I — so it should be dropped rather than worked around. Keep the protected-access one.

Deliberately not in scope: deleting os.close(fd) survives as an untested fd leak (harmless on Linux, the path still exists and copy2 reopens it), and swapping the else: back to an unindented statement while keeping raise also survives — correctly, since the two forms are semantically identical and no test can distinguish them.

Activity

  1. added
    fixPull requests that fix a defect (fix: subject prefix)
    testPull requests that add or correct tests (test: subject prefix)
    ciPull requests that change CI or workflow configuration (ci: subject prefix)
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 28, 2026
  2. removed
    wipWork in flight - a covering PR is open or an agent is actively on it
    on Sep 28, 2026
  3. added this to the 1.5 milestone on Oct 6, 2026
  4. moved this to Done in PyPCAPKiton Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    ciPull requests that change CI or workflow configuration (ci: subject prefix)fixPull requests that fix a defect (fix: subject prefix)testPull requests that add or correct tests (test: subject prefix)

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions