Conversation
Split and modernize DB helpers: introduce tf_sql_connect_update / tf_sql_connect_dict and replace the old toolforge_tools_sql_connect; add mdwiki_sql_update and mdwiki_sql_dict and update callers across the codebase. Add TABLE_NAME constants and tighten typing for many functions. Refactor NewApi: rename/clarify methods (search->api_search, usercontribs->user_contribs), add type hints, improve params handling and add move implementation. Enhance page model (MainPage / pages dataclasses): stronger typing, revisions update via update_from_edit, bot edit validation, and many helper methods reorganized. Misc: package __init__ additions, small naming/formatting tweaks and pyright annotations.
Apply consistent snake_case naming, add type hints, and strengthen defaults across the codebase. Environment variables now fallback to empty strings to avoid None issues. Rename several WD API helpers (Get_sitelinks_From_Qid/Get_Claims_API/Get_claim/WD_Merge -> get_sitelinks_from_qid/get_claims_api/get_claim/wd_merge) and unify request signatures with typed params. Enhance NewApi: search returns list[dict], upload returns a success dict and a new get_page_info_from_wikipedia helper. Increase load_main_api cache size and tighten exception chaining in replaceExcept. Miscellaneous logging/typing fixes for clarity and safety.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MrIbrahem
left a comment
There was a problem hiding this comment.
Review summary
Large mechanical refactor (split DB helpers + snake_case renames + type annotations). Intent is good and most edits are consistent, but the core DB helper is broken and several renamed callers were left dangling.
Verdict: Comment 💬 (forced — you're the PR author; underlying assessment is 🔴 blocking issues found)
Title
Current: Refactor DB access, API naming, and enhance type safety
- The title is accurate. Optional: prefix with
refactor:to match the recent conventional-commit style seen in merged PRs (e.g.refactor: split DB helpers and rename API methods to snake_case). Not required.
Highlights
- Consistent snake_case rename across the API surface (
Get_All_pages→get_all_pages,search→api_search,Get_claim→get_claim, etc.). - Good defensive fix:
params.pop("apnamespace", None)instead ofdel(avoidsKeyError). - New
is_bot_edit_invalidguard blocks bot edits with empty summaries — sensible safety check. - Type annotations added broadly without restructuring logic.
Issues found
- 🔴 Blocking: 4 (see inline comments + the items below that fall outside the diff hunks).
- 🟡 Suggestions: 3
Blocking (details)
src/db/mdapi_sql/sql_td_bot.py—DbConfig.to_dict()raisesKeyError: 'conv'on every call. Theconvkey was removed from the dict literal but is still accessed on the line before it's created. Every DB query in this PR goes through_load_db_config().to_dict(), so all DB access crashes. Even after fixing the crash, the DATE→str converter is lost becausedata["conv"]is overwritten on the next line after the override is applied.tf_sql_connect_updatereturnsFalseon the success path (the onlyreturnstatements arereturn False), so every INSERT/UPDATE/DELETE reports failure regardless of outcome.- Renamed/removed methods still referenced in this PR's own touched files (not inline-commentable because they sit outside the diff hunks — please fix):
src1/md_core/mdpy/red.py:210,243,245→api_new.Search,api_new.Get_Newpages,mdwiki_api_call.Get_UserContribssrc1/md_core/mdpy/imp.py:163,196→api_new.Search,api_new.Get_Newpagessrc1/newupdater/medask.py:131,149,151→api_new.Search,api_new.Get_Newpages,mdwiki_api_call.Get_UserContribsapi_searchalso dropped thereturn_dictparameter —replace1.py:127/one_job.py:168still passreturn_dict=False, which is now an invalid kwarg (TypeError). Andapi_searchnow always returns dicts (full search hits), not title strings, so any downstream code expecting strings must be updated.
sql_for_mdwiki.select_md_sqlwas deleted but is still used in untouched files:src1/md_core/mdpy/orred.py:55andsrc1/copy_text/text_bot.py:21→AttributeError.
Suggestions
- 🟡 Add a smoke test that imports
sql_td_botand callsto_dict()(and ideally opens a connection) — theKeyErrorwould have been caught immediately. - 🟡 After a repo-wide rename,
grepfor the old names to catch dangling callers (thesrc1/breakage above). - 🟡 The repeated
params: dict[str, Any] = {...}local-variable annotations are valid but noisy; reverting to plainparams = {...}is optional.
Summary
Refactors database access, renames API entry points from PascalCase to snake_case, and adds type annotations across the codebase. The generic
mdwiki_sql/toolforge_tools_sql_connecthelpers are replaced by dedicatedmdwiki_sql_update(writes) andmdwiki_sql_dict(reads) functions, and legacyselect_md_sqlis removed.Changes
wiki_sql_connect/toolforge_tools_sql_connectintotf_sql_connect_updateandtf_sql_connect_dict; replacemdwiki_sql(...)/select_md_sql(...)withmdwiki_sql_update(...)/mdwiki_sql_dict(...)insql_for_mdwikiand all callers.Get_All_pages→get_all_pages,search→api_search,Get_claim→get_claim,Get_sitelinks_From_Qid→get_sitelinks_from_qid,WD_Merge→wd_merge,Get_Claims_API→get_claims_api,usercontribs→user_contribs,get_pageassessments→get_page_assessments,isRedirect→isredirect,Get_tags→get_tags, etc.dict[str, Any]/list[...]annotations throughoutsrc/db,src/newapi,src/md_core*,src/wprefs, and markos.getenv(...)reads withor ""defaults.super_page.MainPage: addis_bot_edit_invalidguard (blocks bot edits with empty summaries); minor reordering of helper methods.bot_api: addget_page_info_from_wikipedia; defensiveparams.pop("apnamespace", None).Testing
No tests included.