Skip to content

Refactor (packages/llm/src/schema/options.ts): Function with high complexity - #63

Open
leoliu012 wants to merge 4 commits into
CMU-313:mainfrom
leoliu012:p1b-merge-json-records
Open

Refactor (packages/llm/src/schema/options.ts): Function with high complexity#63
leoliu012 wants to merge 4 commits into
CMU-313:mainfrom
leoliu012:p1b-merge-json-records

Conversation

@leoliu012

@leoliu012 leoliu012 commented Sep 5, 2026

Copy link
Copy Markdown

P1B: Starter Task: Refactoring PR

Use this pull request template to briefly answer the questions below in one to two sentences each.
Feel free to delete this text at the top after filling out the template.

1. Issue

Link to the associated GitHub issue:
Issue 62

Full path to the refactored file:

packages/llm/src/schema/options.ts

What do you think this file does?

This file defines schema-related options and helper functions for the LLM package. mergeJsonRecords combines JSON-like option records while preserving nested values and ignoring undefined values.

What is the scope of your refactoring within that file?

I refactored mergeJsonRecords and extracted the nested value-merging and record-merging logic into mergeJsonRecordValue and mergeJsonRecordInto.

Which Qlty‑reported issue did you address?

Qlty reported Function with high complexity (count = 21): mergeJsonRecords at line 6.

2. Refactoring

How did the specific issue you chose impact the codebase’s maintainability?

mergeJsonRecords handled filtering, special cases, iteration, nested-record detection, and recursive merging in one function, which made the control flow harder to understand and modify safely.

What changes did you make to resolve the issue?

I extracted the logic for merging individual values into mergeJsonRecordValue and the logic for merging one record into mergeJsonRecordInto, leaving mergeJsonRecords responsible for the overall merge flow.

How do your changes improve maintainability? Did you consider alternatives?

The refactoring separates the merge behavior into smaller functions with clearer responsibilities while preserving the existing API and behavior. I considered rewriting the merge using a different iteration pattern, but chose extraction because it reduced complexity with a smaller and less risky behavioral change.

3. Validation

How did you validate that the change is correct?

I ran the tests covering mergeJsonRecords, including nested recursive merges, undefined handling, and single-record behavior, and then ran the package/repository test and lint commands.

Here are the tests added:

describe("mergeJsonRecords", () =>{
  test("returns undefined when all inputs are undefined", () => {
    expect(mergeJsonRecords(undefined, undefined)).toBeUndefined()
  })

  test("returns a single defined record unchanged", ()=>{
    const input = {a:1, b: "test"}

    expect(mergeJsonRecords(input)).toBe(input)
  })

  test("recursively merges nested records", () => {
    expect(
      mergeJsonRecords(
        { config: { a: 1 } },
        { config: { b: 2 } },
      ),
    ).toEqual({
      config: {
        a: 1,
        b: 2,
      },
    })
  })

  test("ignores undefined values when merging", () => {
    expect(mergeJsonRecords({ a: 1 }, { a: undefined })).toEqual({
      a: 1,
    })
  })
})
image

Screenshots showing the tests that cover the change passing during CI
image

image

qlty smells --no-snippets packages/llm/src/schema/options.ts screenshot before the fix
image

qlty smells --no-snippets packages/llm/src/schema/options.ts screenshot after the fix
image

Bun test all passed
image

Bun lint runs
It reports pre-existing linkt/type errors in unrelated files. None are introduced by this refactoring.
image

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