Skip to content

Remove the dead execute_parallel and execute_sequential methods - #271

Merged
himanshu231204 merged 3 commits into
OpenAgentHQ:mainfrom
Nitjsefnie-OSC:fix/52-remove-dead-executor-methods
Aug 3, 2026
Merged

Remove the dead execute_parallel and execute_sequential methods#271
himanshu231204 merged 3 commits into
OpenAgentHQ:mainfrom
Nitjsefnie-OSC:fix/52-remove-dead-executor-methods

Conversation

@Nitjsefnie

Copy link
Copy Markdown
Contributor

Summary

Removes Executor.execute_parallel and Executor.execute_sequential, which nothing in the package calls, along with the two unit tests that were their only callers.

Related Issues and Pull Requests

Fixes #52

Changes

Chose deletion over the TODO marker your issue also offered. The precedent is this repo's own: #75 and #77 were both dead-code findings resolved by deletion and merged. There's no TODO-marker precedent to follow, the methods appear in no docs, examples, changelog or release notes, and the package is pre-1.0 with no deprecation convention. Happy to switch to markers if you'd rather keep them as an intended API.

Testing

One correction to the issue worth stating plainly: "never called anywhere" isn't quite right — they had two dedicated unit tests. No production caller exists, which is what makes deletion correct, but tests/unit/test_core/test_engine.py called them at four points, so those tests had to go with the methods or the suite would AttributeError. That's why the unit count drops by exactly two.

Verified as an enumeration rather than a spot check. Post-change, grep -rn "execute_parallel\|execute_sequential" over the whole tree — source, tests, docs, examples, scripts, config — returns exactly one hit: the CHANGELOG entry itself. A hasattr check on the class confirms both are gone.

gate result
pytest tests/unit 1013 passed, 4 skipped (baseline 1015 — the two deleted tests)
full suite w/ coverage 1067 passed, 4 skipped (baseline 1069)
coverage 81.18%, threshold 75. executor.py goes 55 stmts/85% → 33 stmts/100%
mkdocs build --strict pass
python -m build + twine check pass

The soft gates add no new findings — checked at finding level against a base worktree rather than by comparing totals: ruff 224 vs 228 at base, format clean, mypy unchanged, integration 54 passed.

Footer

Generated by Claude Opus 5 (review), Kimi K3 (brief, implementation, verification)

Nitjsefnie and others added 2 commits August 2, 2026 01:11
Neither method has any production caller; the pipeline uses
Executor.gather(). Their only in-tree references were two dedicated
unit tests, which are removed alongside them. Also drops the now-unused
Coroutine import. Closes OpenAgentHQ#52.

Co-Authored-By: Kimi K3 <noreply@kimi.com>
Co-Authored-By: Kimi K3 <noreply@kimi.com>
@himanshu231204

Copy link
Copy Markdown
Member

/oc review pr

@opencode-agent

opencode-agent Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

PR Review: Remove dead execute_parallel and execute_sequential

Verdict: Approve. The change is correct, minimal, and well-scoped.

Verification performed

  • Read both commits (7d04db1, ab5ba9a) in full.
  • grep across the whole tree — the only surviving references are in the CHANGELOG entry (expected).
  • Confirmed the Coroutine import removal is safe: grep Coroutine in executor.py now returns nothing, so it was only used by the deleted methods' type hints.
  • Confirmed the two deleted tests are exactly the parallel/sequential tests and nothing else in the suite references the removed methods.

Assessment

Correctness — The two removed methods were redundant with Executor.gather() (the pipeline's real path). Deleting them plus the now-orphaned Coroutine import is consistent. The PR body correctly corrects the issue's claim: they were tested (in tests/unit/test_core/test_engine.py), which is precisely why the tests had to be removed too, avoiding an eventual AttributeError. The test-count math (1015→1013) matches.

Consistency with conventions:

Minor nits (non-blocking):

  • CHANGELOG entry is quite verbose (backticks, prose, parenthetical). The repo's other entries use a terse **Title** — summary (#ref) format, so consider trimming the parenthetical, e.g. remove Executor.execute_parallel() and execute_sequential() (no production callers; see gather) (#52).
  • No test environment was available in this sandbox (uv/pytest not installed) to run the suite myself; the PR author's self-reported gates (1013 passed, executurutils 100%) are consistent with the small diff, but I couldn't independently rerun.

Both edges (import removal, test removal, changelog) are correct. I'd ship it.

New%20session%20-%202026-08-03T19%3A53%3A46.639Z
opencode session  |  github run

@himanshu231204
himanshu231204 merged commit cb822cb into OpenAgentHQ:main Aug 3, 2026
8 checks passed
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

🎉 Congratulations @Nitjsefnie!

Your pull request has been successfully merged into main. 🚀

Thank you for contributing to OpenAgentHQ and helping improve the project.

We truly appreciate your contribution and hope to see you back with more amazing PRs!

Happy Open Sourcing! ❤️

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.

execute_parallel and execute_sequential are dead code in executor.py

2 participants