Skip to content

Refactor patch parse - #27

Open
dylany3 wants to merge 4 commits into
CMU-313:mainfrom
dylany3:refactor-patch-parse
Open

Refactor patch parse#27
dylany3 wants to merge 4 commits into
CMU-313:mainfrom
dylany3:refactor-patch-parse

Conversation

@dylany3

@dylany3 dylany3 commented Sep 2, 2026

Copy link
Copy Markdown

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.
Screenshot 2026-09-02 at 16 16 01

Attach a screenshot showing the tests that cover the change passing during CI*
Screenshot 2026-09-02 at 19 48 33

Attach a screenshot of qlty smells --no-snippets <full/path/to/file.ts> showing fewer reported issues after the changes.
Screenshot 2026-09-02 at 19 49 22

Attach a screenshot of bun lint and bun test passing locally (bun test only needs to be run for the package with code changes)
Screenshot 2026-09-02 at 19 50 06
Screenshot 2026-09-02 at 19 50 17

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