-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat(ng-dev/release): add marker between generated changelog entries #212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! just a few comments/questions (especially because this logic will be core to all Angular repos)
const joinMarker = `\n\n${splitMarker}\n\n`; | ||
|
||
/** A RegExp matcher to extract the version of a changelog entry from the entry content. */ | ||
const versionAnchorMatcher = new RegExp(`<a name="(.*)"></a>`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit unhappy about the reliance on some template-specifics here. It will become problematic if we make changes of the anchor. e.g. even when we switch from <a name
to <a id
(which is actually better for fragment resolution in w3c).
If you see my comment on moveEntriesPriorToVersionToArchive
, an option is that we might not need the version at all, or alternatively we could incorporate the version in a specific comment that is generated here as part of the Changelog
class (that keeps the logic more local and self-contained)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include a metadata comment object? Something like.
<!--ReleaseMetadata {version: '1.2.3', name: 'release name'} -->
Then we can retrieve it with a regex and parse the results. We can then JSON.parse
the matched content into something like Partial<ReleaseMetadata>
where we have
interface ReleaseMetadata {
version: string;
name: string;
}
const metadataMatcher = RegExp(/<!--ReleaseMetadata.*(\{.*\})/);
for (const entry of entries) {
const metadata = JSON.parse(entry.match(metadataMatcher)[1]) as Partial<ReleaseMetadata>;
}
There would need to be more validation/assertions I suppose, but this concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, something around that is what I'd love to see. Could also be just something like:
<!-- ReleaseMetadata: Version|Name -->
Doesn't necessarily need to be JSON. I'm happy with either thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to use JSON since it will make parsing easier. It won't be as whitespace sensitive and is easy to roll and unroll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually as I am looking at it now, I think it kind of ends up being a lot more change than we currently have to set this up, how do you feel about doing it in a follow up PR? It just feels like it ends up nearly unrelated to the current change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, works for me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating tracking issue at: #219
44f4a82
to
e0233a6
Compare
0065b53
to
0af0e4c
Compare
Adding a marker `<!-- CHANGELOG SPLIT MARKER -->` between changelog entries allows for easier automated modification of the changelog file, for things like removing prerelease changelog entries when publishing a new major, or automatically moving old changelog entries to an archive changelog.
0af0e4c
to
c1f5098
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Adding a marker
<!-- CHANGELOG SPLIT MARKER -->
between changelog entries allows for easierautomated modification of the changelog file, for things like removing prerelease changelog entries
when publishing a new major, or automatically moving old changelog entries to an archive changelog.
Fixes #198