mikegoatly / lifti

A lightweight full text indexer for .NET
MIT License
184 stars 9 forks source link

FullTextIndexBuilder can only be built once #121

Open mikegoatly opened 3 months ago

mikegoatly commented 3 months ago

As alluded to in #117, a FullTextIndexBuilder doesn't really follow the best practice for builders which is for each action to return a clone of the previous instance with the changes applied.

At the moment this won't do what some might expect:

var coreBuilder = new FullTextIndexBuilder<int>();

var index1 = coreBuilder.WithIndexModificationAction(async (idx) => // Do something).Build();

var index2 = coreBuilder.WithIndexModificationAction(async (idx) => // Do something).Build();

Because the coreBuilder instance will end up with two index modification actions when index2 is built.

Either:

h0lg commented 3 months ago

FullTextIndexBuilder doesn't really follow the best practice for builders which is for each action to return a clone of the previous instance with the changes applied.

I didn't know how to express it that well, but that was my unthinking assumption. But I'm not sure whether returning a modified clone is even a well-established pattern outside the functional world. In non-functional .Net land, I have come across libraries with fluent configuration APIs to modify an instance - so such behaviour is to be expected.

I guess what tripped me up was that reusing the same builder instance worked just fine - until I introduced saving each index using the .WithIndexModificationAction like in your example without creating new builder instances (unlike your example).

mikegoatly commented 3 months ago

Thanks @h0lg - I think it just feels like bad API design if you're not guided down the best path, even if it's by virtue of an exception thrown at runtime.