laimis commented on PR #847: URL: https://github.com/apache/lucenenet/pull/847#issuecomment-1528876126
Alright, thanks for the feedback, really useful! I ended up including DirectoryTaxonomyReader in this PR, in addition to the writer. Adjusted the description for that. This edit pass also includes more XML docs updates, more comments as well. I did not end up doing the IndexFactory suggested updated for several reasons: - the change surface increases quite a bit if we go that route, trying to keep the PRs useful while addressing the issues but at the same time not changing too many things at the same time - I went through with the exercise initially to use separate indexconfig and indexwriter factories just to see what it would look like and it felt awkward, as the DirectoryTaxonomyWriter ties the two together. Adding all the additional constructors to account for permutations of writerfactory and config factory also do not seem to add much value just increase the surface area that needs to be maintained and documented. - conceptually how TaxonomyDirectoryWriter intends to use config and writer is quite different from let's say AnalyzingInfixSuggester. The latter requires analyzer, the former ignores it. - AnalyzingInfixSuggester is second to last place where we need to eliminate virtual method call and with its specific way of using the writer, we should keep that logic tied to it vs pushing to the core of Lucene.Net.Index namespace. Hopefully these make sense! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
