Skip to content

Conversation

@lyrixx
Copy link
Member

@lyrixx lyrixx commented Nov 20, 2025

Q A
Bug fix? no
New feature? yes
Docs?
Issues Fix #875
License MIT

@carsonbot carsonbot added Feature New feature Store Issues & PRs about the AI Store component Status: Needs Review labels Nov 20, 2025
@lyrixx
Copy link
Member Author

lyrixx commented Nov 20, 2025

Before fixing test & co, is this the way to go?

private VectorizerInterface $vectorizer,
private StoreInterface $store,
string|array|null $source = null,
EmbeddableDocumentInterface|array|null $document = [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EmbeddableDocumentInterface|array|null $document = [],
EmbeddableDocumentInterface|array|null $documents = [],

?

}

public function withSource(string|array $source): self
public function withSource(EmbeddableDocumentInterface|string|array $source): self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we accept the EmbeddableDocumentInterface here too?

return new self($this->loader, $this->vectorizer, $this->store, $source, [], $this->filters, $this->transformers, $this->logger);
}

public function withDocument(EmbeddableDocumentInterface|array $document): self
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function withDocument(EmbeddableDocumentInterface|array $document): self
public function withDocuments(array $documents): self

?

@OskarStark
Copy link
Contributor

Lets wait for @chr-hertel here

@chr-hertel
Copy link
Member

with LoaderInterface::load(?string $source, array $options = []): iterable we have something that provides documents, you have documents at hand.

without a source provided, that implementation will be called with $source: null - could you use that?


but that's basically just another idea, but i just struggle with state in constructors - Oskar knows about that flaw in my mind already :D

I think that solution is alright here, I would just omit the EmbeddableDocumentInterface in the withSource or drop the withDocuments - having both feels ambiguous to me - or is it not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature New feature Status: Needs Review Store Issues & PRs about the AI Store component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Store] Indexer is nice, but hard to use directly

4 participants