Refactor (script/beta.ts): Function with high complexity (count = 36): main - #66
Open
horsepaperfish wants to merge 2 commits into
Open
Refactor (script/beta.ts): Function with high complexity (count = 36): main#66horsepaperfish wants to merge 2 commits into
horsepaperfish wants to merge 2 commits into
Conversation
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.
P1B: Starter Task: Refactoring PR
1. Issue
Link to the associated GitHub issue: #58
Full path to the refactored file: /script/beta.ts
What do you think this file does?
Beta.ts collects all open PRs labeled beta, merges them one at a time onto a new branch, delegates any merge conflicts to opencode, and force-pushes the result after a typecheck-and-build smoke check.
What is the scope of your refactoring within that file?
The refactoring is limited to main() and the functions extracted from it. I pulled the PR fetching, branch reset, per-PR merge loop, summary output, and beta sync check out of main() into fetchPRs, reset, merge, commit, apply, run, report, and syncState, and replaced the repeated inline failure handling with an Outcome union and a fail() constructor. This helped to reduce its cyclomatic complexity below the threshold.
Which Qlty‑reported issue did you address?
script/beta.ts:219: Function with high complexity (count = 36): main()
2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
The main() function did everything inline: fetching PRs, resetting the branch, the per-PR merge loop, all six failure paths within that loop, the summary output, and the beta sync check. Moreover, every failure case repeated the same four-line pattern (log, push to failed, comment on the PR, continue) so adding a new failure mode meant duplicating that block again and getting all four steps right.
What changes did you make to resolve the issue?
I extracted the complexities into sub-functions: fetchPRs, reset, merge, commit, apply, run, report, and syncState. The repeated failure handling became an Outcome discriminated union (applied / skipped / failed) with a fail() constructor, so each failure path returns a value instead of duplicating the same four-line pattern.
How do your changes improve maintainability? Did you consider alternatives?
The main() went from 36 decision points to 6, with no extracted function above 8. The Outcome union makes the reason/comment distinction explicit in the type rather than implicit in duplicated code, so adding a new failure mode is a single return fail(...).
Some alternatives: I considered only extracting the duplicated sync check and leaving the loop in main, but that wouldn't have cleared the threshold of reducing the complexity by a marginal amount or removed the repeated failure blocks. I also considered throwing a custom error type instead of returning a value, but rejected it since these are expected outcomes the loop recovers from, not exceptions.
3. Validation
How did you validate that the change is correct?
I Added script/beta.test.ts with 13 unit tests. Each of the unit tests lead to each Outcome variant through a contract apply and assert the resulting applied/failed arrays. I then confirmed these tests passed locally with bun coverage and bun lint unchanged from main, and qlty smells no longer reports the issue.
Attach a screenshot of the test coverage showing the lines were executed by the tests.


Test coverage of beta.test.ts (unit tests I wrote):
Test coverage of bun test coverage:
Attach a screenshot showing the tests that cover the change passing during CI

Attach a screenshot of


qlty smells --no-snippets <full/path/to/file.ts>showing fewer reported issues after the changes.Before changes:
After changes:
Screenshots of bun lint & bun test passing locally



Bun lint On main branch:
Bun lint On fix-beta-complexity branch:
Bun test: