Skip to content

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Aug 22, 2025

@isoos isoos requested a review from sigurdm August 22, 2025 13:42
@isoos
Copy link
Collaborator Author

isoos commented Aug 22, 2025

Note: maybe we should also remove < and > characters from the excerpt, so that we don't accidentally create a HTML tag in the email?

Update: Implemented a simple string replace which should be probably enough.

final indent = ' ' * (_listDepth - 1);
_write(indent);
if (parent == 'ol') {
_write('1. ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we attempt counting here?

!line.startsWith('---'))
.take(10)
// prevent accidental HTML-tag creation
.map((line) => line
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do &lt;-style escaping? Would that work in an email?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could have emails with both text and HTML versions, but that would complicate things way too much. I'd rather focus on the text content here.

return excerpt;
} catch (e, st) {
_logger.pubNoticeShout('changelog-parse-error',
'Unable to parse changelog for $versionKey', e, st);
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this log the package name also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it is <package>/<version>.

contains('\n'
'Excerpt of the changelog:\n'
'```\n'
'Some bug fixes:\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider also including the header of the relevant section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure. For most packages, it will be only the version string. Maybe if the changelog itself is not too long?

.map((line) =>
line.length < 76 ? line : '${line.substring(0, 70)}[...]')
.join('\n');
return excerpt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return null if the excerpt is empty/only whitespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are filtering out most of the empty spaces and lines, maybe empty list items will be still kept. I'll add a bit more conditions to prevent / post-filter those.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking if the "whole message" is whitespace only - it would look weird in the email.

Here's an excerpt from the CHANGELOG:

And now for something completely different... 

@isoos isoos merged commit 7b871a4 into dart-lang:master Aug 25, 2025
31 checks passed
@isoos isoos deleted the changelog-email branch August 25, 2025 19:36
@kevmoo
Copy link
Member

kevmoo commented Aug 25, 2025

"markdown -> html -> changelog parser -> html content of the release -> markdown-ified version of the release -> taking only 10 lines, with limited character counts." – WOW!

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.

Feature request: include change log in "Package uploaded" emails

3 participants