Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: SUMMARY_MAX_PARAGRAPHS not respected in some combinations with SUMMARY_MAX_LENGTH #3427

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

scolby33
Copy link
Contributor

@scolby33 scolby33 commented Nov 23, 2024

When, for example, the content is 2 paragraphs, each consisting of 30 words, and SUMMARY_MAX_PARAGRAPHS=1 and SUMMARY_MAX_LENGTH=50, the current behavior produces a summary with 50 words and 2 paragraphs, effectively ignoring SUMMARY_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

  • Ensured tests pass and (if applicable) updated functional test output
  • Conformed to code style guidelines by running appropriate linting tools
  • Added tests for changed code
  • Updated documentation for changed code no need for documentation changes

@justinmayer
Copy link
Member

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?

When, for example, the content is 2 paragraphs, each consisting of 30
words, and SUMMARY_MAX_PARAGRAPHS=1 and SUMMARY_MAX_LENGTH=50, the
current behavior produces a summary with 50 words and 2 paragraphs,
effectively ignoring SUMMARY_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.
@scolby33
Copy link
Contributor Author

@justinmayer no problem, changed the release type in the latest version!

@gagath
Copy link
Contributor

gagath commented Nov 24, 2024

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 justinmayer changed the title Fix SUMMARY_MAX_PARAGRAPHS not being respected in some combinations with SUMMARY_MAX_LENGTH. fix: SUMMARY_MAX_PARAGRAPHS not respected in some combinations with SUMMARY_MAX_LENGTH Nov 27, 2024
Copy link
Member

@justinmayer justinmayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks to @scolby33 for the enhancement and to @cthoyt & @gagath for reviewing! 🏅

@justinmayer justinmayer merged commit d9652ef into getpelican:main Nov 27, 2024
16 checks passed
@cthoyt
Copy link

cthoyt commented Nov 27, 2024

@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!

@justinmayer
Copy link
Member

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! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants