cal/major-domo-v2
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix: replace create_item_in_table placeholder with direct endpoint call (#30) #69

Merged
cal merged 1 commits from ai/major-domo-v2#30 into main 2026-03-07 03:25:09 +00:00
Conversation 0 Commits 1 Files Changed 2 +175 -162
cal commented 2026-03-06 20:04:39 +00:00
Owner
Copy Link

Summary

  • Removes the create_item_in_table placeholder method from BaseService — it existed solely to POST to an arbitrary table_name string with a comment acknowledging it wasn't a real implementation
  • Replaces the single call site in CustomCommandsService.get_or_create_creator with a direct client.post("custom_commands/creators", creator_data) call, consistent with how _update_creator_stats and _update_creator_info already interact with the creators sub-endpoint

Files Changed

  • services/base_service.py — removed 22-line placeholder method; note: project linter auto-reformatted the file (quote style, line length) as a side effect of the edit
  • services/custom_commands_service.py — replaced 3-line create_item_in_table call with 2-line direct client POST

Test Results

926 passed, 1 skipped (6 pre-existing failures in test_commands_profile_images.py unrelated to this change)

Other observations

  • get_items_from_table_with_params in base_service.py is a similar generic-table helper used exclusively by custom_commands_service.py for creator reads. It could be refactored into a dedicated _get_creators method in CustomCommandsService in a follow-up, but is out of scope for this issue.
## Summary - Removes the `create_item_in_table` placeholder method from `BaseService` — it existed solely to POST to an arbitrary `table_name` string with a comment acknowledging it wasn't a real implementation - Replaces the single call site in `CustomCommandsService.get_or_create_creator` with a direct `client.post("custom_commands/creators", creator_data)` call, consistent with how `_update_creator_stats` and `_update_creator_info` already interact with the creators sub-endpoint ## Files Changed - `services/base_service.py` — removed 22-line placeholder method; note: project linter auto-reformatted the file (quote style, line length) as a side effect of the edit - `services/custom_commands_service.py` — replaced 3-line `create_item_in_table` call with 2-line direct client POST ## Test Results 926 passed, 1 skipped (6 pre-existing failures in `test_commands_profile_images.py` unrelated to this change) ## Other observations - `get_items_from_table_with_params` in `base_service.py` is a similar generic-table helper used exclusively by `custom_commands_service.py` for creator reads. It could be refactored into a dedicated `_get_creators` method in `CustomCommandsService` in a follow-up, but is out of scope for this issue.
cal added 1 commit 2026-03-06 20:04:40 +00:00
fix: replace create_item_in_table placeholder with direct endpoint call (#30)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m10s
Details
18ae65a8e2 
Remove the generic placeholder method from BaseService and replace the
single call site in CustomCommandsService.get_or_create_creator with a
direct client.post("custom_commands/creators", ...) call, consistent
with how _update_creator_stats and _update_creator_info already work.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal added the
ai-reviewing
 label 2026-03-06 20:15:35 +00:00
cal reviewed 2026-03-06 20:16:43 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • services/base_service.py (modified — removed create_item_in_table, linter reformatting)
  • services/custom_commands_service.py (modified — replaced call site in get_or_create_creator)

Findings

Correctness

  • The removed create_item_in_table method did exactly what the replacement does: client.post(table_name, item_data). The call site change is semantically equivalent.
  • The new code drops the try/except wrapper that create_item_in_table had (which caught Exception and returned None). Now exceptions from client.post() propagate directly. The caller already handles None by raising BotException, so any exception path still terminates with an exception — this is actually better because the original error details are preserved rather than being swallowed into a None return.
  • The "custom_commands/creators" endpoint string is consistent with the strings used in _update_creator_stats (line 747), _update_creator_info (line 761), and get_creator_by_discord_id (line 572).

Security

  • No issues. The endpoint string is hardcoded (not user-supplied), and creator_data is constructed from validated method parameters.

Style & Conventions

  • The self.get_client() + client.post() pattern within CustomCommandsService is established throughout the class (see get_command_by_name, execute_command, get_commands_by_creator, _update_creator_stats, _update_creator_info). The replacement is consistent with these peers.
  • The services/CLAUDE.md anti-pattern ("reject any PR that calls await service.get_client() outside of service layer") targets commands/tasks/views bypassing the service layer — not service internals calling self.get_client(). No violation here.
  • Linter reformatting (single→double quotes, line length) is a clean side effect with no semantic impact.

Suggestions

  • The PR author's note about get_items_from_table_with_params being a similar candidate for refactoring is accurate and appropriate to defer. No action needed now.

Verdict: COMMENT

Clean removal of a placeholder. The implementation is correct, follows established patterns within CustomCommandsService, and improves error transparency by letting API exceptions propagate with full context rather than silencing them into a None return. No issues blocking merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/base_service.py` (modified — removed `create_item_in_table`, linter reformatting) - `services/custom_commands_service.py` (modified — replaced call site in `get_or_create_creator`) ### Findings #### Correctness - The removed `create_item_in_table` method did exactly what the replacement does: `client.post(table_name, item_data)`. The call site change is semantically equivalent. - The new code drops the `try/except` wrapper that `create_item_in_table` had (which caught `Exception` and returned `None`). Now exceptions from `client.post()` propagate directly. The caller already handles `None` by raising `BotException`, so any exception path still terminates with an exception — this is actually better because the original error details are preserved rather than being swallowed into a `None` return. - The `"custom_commands/creators"` endpoint string is consistent with the strings used in `_update_creator_stats` (line 747), `_update_creator_info` (line 761), and `get_creator_by_discord_id` (line 572). #### Security - No issues. The endpoint string is hardcoded (not user-supplied), and `creator_data` is constructed from validated method parameters. #### Style & Conventions - The `self.get_client()` + `client.post()` pattern within `CustomCommandsService` is established throughout the class (see `get_command_by_name`, `execute_command`, `get_commands_by_creator`, `_update_creator_stats`, `_update_creator_info`). The replacement is consistent with these peers. - The `services/CLAUDE.md` anti-pattern ("reject any PR that calls `await service.get_client()` outside of service layer") targets commands/tasks/views bypassing the service layer — not service internals calling `self.get_client()`. No violation here. - Linter reformatting (single→double quotes, line length) is a clean side effect with no semantic impact. #### Suggestions - The PR author's note about `get_items_from_table_with_params` being a similar candidate for refactoring is accurate and appropriate to defer. No action needed now. ### Verdict: COMMENT Clean removal of a placeholder. The implementation is correct, follows established patterns within `CustomCommandsService`, and improves error transparency by letting API exceptions propagate with full context rather than silencing them into a `None` return. No issues blocking merge. --- *Automated review by Claude PR Reviewer*
cal added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-06 20:17:28 +00:00
cal merged commit 111a6cf338 into main 2026-03-07 03:25:09 +00:00
cal deleted branch ai/major-domo-v2#30 2026-03-07 03:25:10 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
in-next-release

Fix included in the next-release branch, pending merge to main

  
status/in-progress

Work is actively in progress

  
status/pr-open

A pull request has been opened for this issue

No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
in-next-release
status/in-progress
status/pr-open
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-v2#69
No description provided.
Delete Branch "ai/major-domo-v2#30"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?