Closed scolby33 closed 3 days ago
Thank you, Scott, for submitting this pull request — replete with a test, no less! ✨
Given the narrow scope of these changes, I think a release type of patch
would be more appropriate. Would you please change the release file accordingly?
@gagath: As the original author of the SUMMARY_MAX_PARAGRAPHS
setting, do you have any comments on this PR?
@justinmayer no problem, changed the release type in the latest version!
Hello @justinmayer and @scolby33, this change looks good to me and indeed fixes an implementation error of SUMMARY_MAX_PARAGRAPHS=1
not being honored if the paragraph length is too short. Thanks for the fix!
@justinmayer not relevant to the PR but @scolby33 and I are flatmates and we thought it was funny we both had random PRs for two of your different projects back to back. Cheers!
No way! 😮 That is indeed quite funny — literally laughed out loud when I read it.
It’s been a pleasure. I hope there will be more random open-source encounters with y’all in the future. If you’re ever in the Italian Alps, beers are on me! 🍻
When, for example, the content is 2 paragraphs, each consisting of 30 words, and
SUMMARY_MAX_PARAGRAPHS=1
andSUMMARY_MAX_LENGTH=50
, the current behavior produces a summary with 50 words and 2 paragraphs, effectively ignoringSUMMARY_MAX_PARAGRAPHS
.This change inverts this behavior: the resulting summary from the above situation will consist of the entire first 30 word paragraph.
In the case where the first paragraph(s) is longer than
SUMMARY_MAX_LENGTH
, the summary will still be truncated to that length.This change brings the actual behavior in line with the documented behavior.
Pull Request Checklist