shubhamvishu commented on PR #12671: URL: https://github.com/apache/lucene/pull/12671#issuecomment-1763076574
Thanks @gsmiller for the review! My motivation behind this refactoring was [this comment](https://github.com/apache/lucene/pull/12548#discussion_r1357027508) from Mike which indicated this file might need some cleanup. Also my idea to refactor the private DVS implementations was: 1. To keep the `DoubleValuesSource` class lean and only use this class to define the public APIs, making it easier to read and not have the specific implementations in it which seem to be cluttering this class (have the API definitions and implementations separate). 2. To promote reuseability of some implementations when needed. Right now these implementations are not used, but could be (eg : `ConstantValuesSource`?) or something that would be added in the future. Further, we could use this new utility class to provide those reusable DVS implementations. I completely understand your point that this refactoring may not seem to be very helpful right now but I feel this looks more cleaner(to separate the APIs and the implementations). I'm thinking if this new class would make sense in future for some sort of a class that provides different DVS reusable implementations to a user. I'm fine either ways and happy to keep it as it is, if we see no need of it at present. Since we do not feel strongly about it, I would like to know others opinion in this as well. Tagging @benwtrent @msokolov to know their thoughts on this too. -- 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: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
