krickert commented on PR #1478: URL: https://github.com/apache/opennlp/pull/1478#issuecomment-6112082459
> Hi @krickert, thanks for the PR. Supplying an own tagger and chunker is a useful addition. Some remarks: > > **API** > > * Devil's advocate: do we need `ParserBuilder`? With four parameters, of which two already have defaults through `ParserFactory.create`, a factory overload `create(model, tagger, chunker, beamSize, advancePercentage)` covers the use case without a new public type, and it follows the existing factory style. A builder can still be added later in 3.x if more options come up; once released, it cannot be removed. I would suggest to drop it and keep the new constructors plus the factory overload. Maybe it's just me, but working with these constructors was a bit annoying. A builder just looked so much cleaner to me. Given that it is 3.x, and that we substantially changed parsing, I was thinking now is a good time to tear off the band aid. I considered making backward compat deprecated helpers as a compromise.. but my favor is builder pattern first.. So we don't need it, but I use parsers a lot and it might fall into "well that's your preference" territory and my personal opinion doesn't mean the standard. But builders are in fashion these days, I think our API looks better if we have it. Large constructors are always questionable to me. 1-2 is fine, but this is larger.. However, not worried about any pushback - just thought I'd give my two cents. I'll yield the decision to you.. it's only a few minutes to go either way. > > **Argument checks** > > * `AbstractBottomUpParser.java`: `checkBeamSize` and `checkAdvancePercentage` are local copies of generic argument checks. Please add `ParamChecks.requirePositive(int, String)` and a check for `(0, 1]` to [opennlp-api](https://issues.apache.org/jira/browse/OPENNLP-api) and use them here. The same positive check exists in `CachingStemmer`, `SnowballStemmerFactory`, `ArcStandardState`, `NGramLanguageModel` and `NGramModel`, and #1323 adds another one, each with its own message. Aligning these can be a separate issue, but the new code should not add further copies. np > * Please use the message format of `ParamChecks` instead of "beamSize must be at least 1, but was 0". > np > **Tests** > > * Only the reject side is tested. Please also pin the accepted boundaries (`beamSize` 1, `advancePercentage` 1.0 and a small positive value). > * The null-argument tests can each be one `@ParameterizedTest`. Once the range checks live in `ParamChecks`, testing them there and keeping one test per entry point is sufficient. > on it > **Docs** > > * `parser.xml`: "Since 3.0.0 the parser can tag and chunk …" is changelog wording; please describe the current behaviour without the version reference. > Sorry, I thought I got rid of those corners.. > **Behaviour** > > * `ParserFactory.create` and the existing `Parser` constructors now reject a beam size below 1 and an advance percentage outside `(0, 1]`. Please add this to the README breaking-change list. np -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
