Skip to content

feat: add database batch iterator#21

Draft
Ramlaoui wants to merge 1 commit into
mainfrom
codex/feat/database-iter-batches
Draft

feat: add database batch iterator#21
Ramlaoui wants to merge 1 commit into
mainfrom
codex/feat/database-iter-batches

Conversation

@Ramlaoui
Copy link
Copy Markdown
Collaborator

@Ramlaoui Ramlaoui commented May 9, 2026

No description provided.

@Ramlaoui Ramlaoui force-pushed the codex/feat/database-iter-batches branch from ec5e7f1 to 49cefa3 Compare May 10, 2026 13:21
@speckhard
Copy link
Copy Markdown
Contributor

speckhard commented May 13, 2026

Had claude take a look this is what popped up:


**Monkey-patching a Rust class from `__init__.py` is surprising.** `Database.iter_batches = _database_iter_batches` mutates the PyO3-exposed class at import time. It works (PyO3 classes accept attribute assignment on the type unless `#[pyclass(frozen)]` is set), but it has consequences:
    - Subclasses (e.g. for testing or wrapping) inherit the bound method but cannot easily override it because the function is patched on the class rather than declared in `#[pymethods]`. This is a small inheritance smell.
    - `inspect.getsource(db.iter_batches)` will reveal the pure-Python implementation, not show up next to the other Rust methods in IDEs/docs.
    - Importing `from atompack._atompack_rs import PyAtomDatabase` gets the un-patched class. If anything in the test suite or downstream code does this, `iter_batches` is missing.
    - The `__init__.pyi` declares `iter_batches` as a method of `Database`, so the stub disagrees with the runtime class definition in a subtle way (stub says "method", runtime says "monkey-patched function").

    Alternative options, in order of cleanliness:
    - **(Best)** declare `iter_batches` on the Rust side in `database.rs` as a `#[pymethod]`. It is small enough that this is not a big task.
    - **(Second)** wrap `Database` in a thin Python subclass that adds `iter_batches`, and re-export the subclass. This keeps the Rust class clean.
    - **(Third — what is here)** keep the monkey patch but add a comment in `__init__.py` explaining *why* it is done this way (e.g. "Rust class is exported as-is; thin pure-Python batching helper added here to avoid plumbing kwargs through pyo3").

Otherwise looks quite clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants