Skip to content

[no-impl] Pass a closure to visitDataBlobs - #262

Draft
JayChung0258 wants to merge 2 commits into
mainfrom
jayhung/sa-1-blob-closure
Draft

[no-impl] Pass a closure to visitDataBlobs#262
JayChung0258 wants to merge 2 commits into
mainfrom
jayhung/sa-1-blob-closure

Conversation

@JayChung0258

@JayChung0258 JayChung0258 commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

The main idea

The functions that open a history blob need to hand the events inside to someone. Today that someone only needs one thing, the matcher, so passing the matcher as a parameter works fine.

That is about to stop being true. The next change needs to also pass in the namespace id, and the namespace id is only known at the call site, read off the message just before the blob is opened. If we keep passing things as parameters, all three functions in the chain grow a parameter now, and another one next time.

So instead of the helpers being told what to pass along, the caller hands them a closure and they just call it. The caller decides what to carry in.

What was changed

visitDataBlobs, translateDataBlobs and translateOneDataBlob took a stringMatcher plus a visitor function. They now take one blobVisitor closure. The two callers wrap their existing call in it.

Before:

visitDataBlobs(logger, vwp, match, visitNamespace)

After:

visitDataBlobs(logger, vwp, func(events []*history.HistoryEvent) (bool, error) {
    return visitNamespace(logger, events, match)
})

And this is the call that needs it, in a later PR. Note that match alone is no longer enough:

nsID := resolveNamespaceID(vwp, fallbackNamespaceID)
visitDataBlobs(logger, vwp, func(events []*history.HistoryEvent) (bool, error) {
    return visitSearchAttributes(logger, events, resolve, nsID)
})

One file, no behaviour change. Split out on its own so the change that follows is easier to read.

Checklist

  1. First step toward CGSCE-639.

  2. How was this tested:

make generate-test-certs
go test -race -timeout=12m -tags test_dep -count=1 ./...
make lint

Both clean. No test file changed, which is the main thing I wanted to be true here.

One spot worth a second look. The matched and changed return values are handled exactly as before. Those two together decide whether a blob is written back, and no test tells them apart, so a mistake there would be quiet.

  1. Any docs updates needed?

No.

🤖 Generated with Claude Code

visitDataBlobs, translateDataBlobs and translateOneDataBlob took a
stringMatcher and a visitor function and passed both down to the events
inside the blob. They now take one blobVisitor closure, and the two callers
wrap their existing call in it.

No behaviour change, and no test needed updating.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bv was too short to read.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@JayChung0258 JayChung0258 changed the title Pass a closure to visitDataBlobs [no-impl] Pass a closure to visitDataBlobs Aug 25, 2026
@JayChung0258
JayChung0258 marked this pull request as draft August 25, 2026 02:38
@JayChung0258

Copy link
Copy Markdown
Contributor Author

Parking this. It is a no-op that only pays off in a later PR, which makes it hard to review on its own merits, and we may not need it: a wider parameter on the shared visitor type would also work, and visitNamespace only has four call sites so widening it is cheap.

Leaving the branch up so the closure version is here to compare against when we get to the PR that actually needs it. The decision belongs there, next to its motive.

One thing worth remembering either way: if we go the parameter route, the shared signature should not carry both a stringMatcher and a saMatcherResolver. They are mutually exclusive, so one is always nil and the middle functions stop telling you which arguments matter.

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