Skip to content

fix(etree): return list instead of calling reply per line#25

Open
h4ksclaw wants to merge 2 commits into
h4ks-com:mainfrom
h4ksclaw:fix/etree-accumulate-output
Open

fix(etree): return list instead of calling reply per line#25
h4ksclaw wants to merge 2 commits into
h4ks-com:mainfrom
h4ksclaw:fix/etree-accumulate-output

Conversation

@h4ksclaw
Copy link
Copy Markdown

@h4ksclaw h4ksclaw commented Jun 2, 2026

Problem: .etree was calling reply() for every line of the etymology tree, spamming the channel with individual messages.

Fix: Changed etymology_tree to return a list[str] instead. CloudBot's hook system handles list returns by sending each element as a separate message — same net effect for output, but cleaner internally (no direct reply calls, proper return-value pattern matching the rest of the plugin).

Changes:

  • Removed nick and reply params (unused now)
  • Build the full tree string, split by newline, return as list
  • Returns None if tree is empty (no output)

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

Review Change Stack

Warning

Review limit reached

@h4ksclaw, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 54 minutes. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 20b7f1bd-2c07-44f0-bd9f-fdd350d14a3d

📥 Commits

Reviewing files that changed from the base of the PR and between 55cb5d8 and 2525f3f.

📒 Files selected for processing (1)
  • plugins/taxonomy.py
📝 Walkthrough

Walkthrough

The etymology_tree function in plugins/etymology.py was refactored to simplify its interface: it now accepts only the text parameter, internally computes the etymology tree, and returns the result as a list of split lines (or None if the tree is empty), instead of accepting nick and reply parameters and writing pages through a callback.

Changes

Etymology Tree Command Refactor

Layer / File(s) Summary
Function signature and return-based output
plugins/etymology.py
The etymology_tree function signature changed from (nick, text, reply) to (text). The function now computes ety.tree(text.strip()) and returns the split lines or None, replacing the previous pattern of iterating over pages and invoking a reply callback for each page.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 A rabbit hops through etymology's trees,
Old callbacks fade like leaves in the breeze,
Now functions return what they find with such grace,
Simpler signatures bring order and space! 📚✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: refactoring the etymology_tree function to return a list instead of calling reply for each line.
Description check ✅ Passed The description is directly related to the changeset, explaining the problem, solution, and specific code changes made to the etymology_tree function.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
plugins/etymology.py (1)

23-26: Approve etymology_tree output shaping (empty handling + return-to-chat contract)

  • str(ety.tree(...)) is already newline-trimmed, and EtyTree.__str__ returns "" when treelib can’t render the tree (so tree.strip() correctly maps to None; split("\n") won’t produce a trailing blank line).
  • CloudBot’s @hook.post_hook (plugins/core/core_hooks.py:do_reply) sends command return values to chat (list/tuple -> reply(*result)), so returning list[str]/None (rather than calling reply() inside the handler) matches the framework contract.

No repo tests reference etymology_tree/etree; consider adding coverage for both the empty/unknown case and a non-empty tree case.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/etymology.py` around lines 23 - 26, Add unit tests for etymology_tree
to cover both the empty/unknown case and a non-empty tree case: mock or
instantiate EtyTree so EtyTree.__str__ returns "" to assert etymology_tree(...)
yields None, and a rendered multiline string to assert etymology_tree(...)
returns a list of lines (no trailing empty line); ensure the tests exercise
ety.tree via the etymology_tree function and confirm compatibility with the
framework behavior in do_reply (i.e., function returns list[str] or None rather
than calling reply()).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@plugins/etymology.py`:
- Around line 23-26: Add unit tests for etymology_tree to cover both the
empty/unknown case and a non-empty tree case: mock or instantiate EtyTree so
EtyTree.__str__ returns "" to assert etymology_tree(...) yields None, and a
rendered multiline string to assert etymology_tree(...) returns a list of lines
(no trailing empty line); ensure the tests exercise ety.tree via the
etymology_tree function and confirm compatibility with the framework behavior in
do_reply (i.e., function returns list[str] or None rather than calling reply()).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fb07b790-95ac-48eb-ad62-d51a15dbae9f

📥 Commits

Reviewing files that changed from the base of the PR and between 1ae0bea and 55cb5d8.

📒 Files selected for processing (1)
  • plugins/etymology.py

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.

1 participant