Refactor patch parse - #27
Open
dylany3 wants to merge 4 commits into
Open
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: #26
Full path to the refactored file: packages/core/src/patch.ts
What do you think this file does?
It parses and applies opencode's patch format, the *** Begin Patch / *** Add File: / *** Update File: blocks that the apply-patch tool uses to edit files. parse turns patch text into structured hunks, and derive applies an update hunk's chunks to a file's existing contents.
What is the scope of your refactoring within that file?
Only the parse function and the new helpers extracted from it: parseAddHunk, parseDeleteHunk, parseUpdateHunk, parseHunk, and directivePath. No other function was modified.
Which Qlty‑reported issue did you address?
Function with high complexity (count = 31) in parse at line 25.
2. Refactoring
How did the specific issue you chose impact the codebase’s maintainability?
All three directive types were handled inline in a single while loop, so anyone adding or changing a directive had to read the whole function to find the relevant branch and risked disturbing the shared loop index. The slice-trim-validate-throw pattern was also duplicated four times with only the error message differing.
What changes did you make to resolve the issue?
I extracted each directive branch into its own function returning { hunk, next }, added a parseHunk dispatcher that matches the prefix and delegates, pulled the repeated path validation into a directivePath helper, and replaced the literal prefix strings with named constants.
How do your changes improve maintainability? Did you consider alternatives?
Each directive is now readable and changeable in isolation, and adding a fourth directive means writing one function plus one line in the dispatcher instead of editing the loop body. I considered a lookup table mapping prefixes to handlers, but the startsWith checks are order-sensitive and a table would have obscured that without reducing complexity further.
3. Validation
How did you validate that the change is correct?
packages/core/test/patch.test.ts (6 tests) and packages/core/test/tool-apply-patch.test.ts (9 tests) all pass without any modification to the tests themselves, which is the point: they cover every path the refactor touched, including add, delete, update, the move directive, and malformed-input errors, so unchanged tests passing means unchanged behavior. Coverage on src/patch.ts is 87.50% of functions and 88.05% of lines, and bun lint reports the same 698 warnings and 2 errors as before the change.
Attach a screenshot of the test coverage showing the lines were executed by the tests.

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.Attach a screenshot of bun lint and bun test passing locally (bun test only needs to be run for the package with code changes)

