Skip to content
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

FullTextIndexBuilder can only be built once #121

Open
mikegoatly opened this issue Aug 23, 2024 · 2 comments
Open

FullTextIndexBuilder can only be built once #121

mikegoatly opened this issue Aug 23, 2024 · 2 comments

Comments

@mikegoatly
Copy link
Owner

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:

  • Builder instances need to be immutable
  • An exception should be thrown if Build is called multiple times
  • An exception should be thrown if the builder is mutated after it has been built (this would at least allow for multiple calls of Builds)
  • Something else I haven't thought of yet.
@h0lg
Copy link

h0lg commented Aug 23, 2024

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
Copy link
Owner Author

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.

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

No branches or pull requests

2 participants