Skip to content

Append handlers in-situ. #118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

plx
Copy link

@plx plx commented May 23, 2025

While starting to read the code I noticed some code like this:

let oldValue = someDictionary[key, default:[]]
someDictionary[key] = oldValue + [valueToAppend]

I changed those sites to look like this:

someDictionary[key, default: []].append(valueToAppend)

Motivation and Context

The updated code may sometimes perform an in-situ append on the underlying storage; the original would always construct a new array.

In a worst-case scenario the original code could exhibit accidentally-quadratic behavior while, say, registering a large number of "handlers".

How Has This Been Tested?

I re-ran the unit tests and they still passed.

I also ran this test to double-check that the setter for subscript(key:default:) existed and worked as I thought:

@Test("In-Place Default Append Works")
func testInPlaceDefaultAppend() {
  var inSituAppend: [String: [Int]] = [:]
  var copyThenOverwrite: [String: [Int]] = [:]
  for value in 1...10 {
    for key in ["foo", "bar", "baz"] {
      inSituAppend[key, default: []].append(value)
      let oldValue = copyThenOverwrite[key, default: []]
      #expect(inSituAppend != copyThenOverwrite)
      copyThenOverwrite[key] = oldValue + [value]
      #expect(inSituAppend == copyThenOverwrite)
    }
  }
}

I didn't include that test in the PR because it's a one-off experiment to re-confirm my understanding of a standard-library method, but I'm happy to put it back in if you'd like.

Breaking Changes

No.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Glad there's an official sdk!

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