magibney commented on pull request #380: URL: https://github.com/apache/lucene/pull/380#issuecomment-1005887111
Apologies for the delay, and thanks for bearing with me, @spyk. I'm inclined to err on the cautious side with this, since I'm not as familiar with this part of the codebase or the OpenNLP community. That said, this seems really straightforward to me, and clearly an improvement. (I considered, but decided against, suggesting to adopt `computeIfAbsent()` in place of `cached = map.get(...); if (cached==null) map.put(...)` ... the former is "newer" and semantically clearer, but the latter is more idiomatic to this part of the codebase). The only thing giving me pause now is that I notice we're changing the return type of a _public_ method. If there are third-party extensions that rely on the existing return type of this method, they will break. An easy fix, but still ... I'm additionally chastened to see that the issue introducing this code, [LUCENE-2899](https://issues.apache.org/jira/browse/LUCENE-2899), has 36 "votes" and 68 "watchers" (!) -- so the chance of this being a breaking change for some third party extension are not insignificant (FWIW I'd be surprised if third parties actually called this method in practice, but I'm not sure I have the perspective to judge :slightly_smiling_face:) I dislike the idea of maintaining backward compatibility "just because", when this seems like such a clear improvement, and when I suspect that the `public` access for these static methods may not necessarily represent an explicit design choice (?); and with the 9.0 release still quite fresh, arguably now would not be the worst time to break backcompat (esp. in such a minor way). But I'm afraid I really would like another committer (ideally, @sarowe?) to weigh in on this. Thank you for your patience! -- 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
