Repository navigation
Conversation
bjester
added this pull request to stack #402
October 23, 2026 20:47
bjester
marked this pull request as ready for review
October 23, 2026 20:51
The streaming deserialization pipeline (#392) replaced the only callers of these. Removes SyncableModel.deferred_clean_fields, ForeignKeyReference, TemporaryTable, and the backend _bulk_full_record_upsert and _bulk_update helpers, along with the helpers that only they used: _bulk_insert, _create_temporary_table, _create_placeholder_list, the PostgreSQL _prepare_* methods and get_pk_field. calculate_max_sqlite_variables is kept because Kolibri imports it. Closes #396 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
bjester
force-pushed
the
remove-cruft-396
branch
from
October 23, 2026 21:35
75173ee to
bd4e642
Compare
Contributor
🟡 Waiting for changesOutcome: reviewed Last updated: 2026-10-23 22:23 UTC |
rtibblesbot
reviewed
Oct 23, 2026
rtibblesbot
left a comment
rtibblesbot
left a comment
Contributor
There was a problem hiding this comment.
PR #401 is a clean removal: no references to the removed names remain in morango/ or tests/. The extra helpers beyond #396's list served only the removed code. The changelog names them.
CI pending.
- suggestion: Remove
calculate_max_sqlite_variables; it has no callers left (inline).
Next step: Needs a full code review by a human engineer.
- Confirm no downstream consumer (e.g. Kolibri) imports
TemporaryTable,get_pk_field,deferred_clean_fieldsor the backend_bulk_*helpers before they are removed.
Written by rtibblesbot, an LLM-based coding agent.
| SQLite has a limit on the max number of variables allowed for parameter substitution. This limit used to be 999, but | ||
| can be compiled to a different number, and is now often much larger. This function reads the value from the compile options. | ||
| We use this value to chunk our SQL bulk insert statements when deserializing from the store to the app layer. | ||
| Callers use this value to chunk SQL statements so they stay within the limit. |
Contributor
There was a problem hiding this comment.
suggestion: calculate_max_sqlite_variables has no callers after this PR. Its only callers were the sqlite _bulk_* methods this PR removes. This docstring line still describes those callers. Remove the function and the two imports only it uses. If it is kept for external use, say so in the docstring.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes the deferred FK validation API and the backend bulk write helpers. The streaming deserialization pipeline from #392 replaced their only callers.
This PR removes more than the five items in #396. After those items were gone, these helpers had no callers left, so this PR removes them too:
_bulk_insert,_create_temporary_tableandcreate_temporary_table_templatein the backends_create_placeholder_listin the base backend_prepare_with_values,_prepare_casted_fieldsand_prepare_set_casted_valuesin the PostgreSQL backendget_pk_fieldinmorango/sync/backends/utils.pyA search of
learningequality/kolibrifound no use of any removed name.calculate_max_sqlite_variablesstays. The removed SQLite helpers called it, but Kolibri imports it in three modules.TODO
test_deferred_clean_fields, which only tested removed code.0.9.0section ofCHANGELOG.mdlists the removed names.Reviewer guidance
This PR is based on
reformatting-394. That branch removes the ruff exclude list, so ruff now finds unused imports inpostgres.pyandsqlite.py.The full test suite passes on SQLite and PostgreSQL.
Issues addressed
Closes #396
AI usage
I used Claude Code to find the remaining callers in Morango and Kolibri, remove the code, and run the tests on both databases. Claude Code found the extra unused helpers, and I chose to remove them in this PR. I reviewed the diff before I opened the PR.
🤖 Generated with Claude Code