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: prevent duplicate headers from being added (#305) #307

Merged

Conversation

jthomerson
Copy link
Contributor

@jthomerson jthomerson commented Feb 25, 2019

Fixes #305

@@ -5,6 +5,7 @@ const conventionalChangelog = require('conventional-changelog')
const fs = require('fs')
const runLifecycleScript = require('../run-lifecycle-script')
const writeFile = require('../write-file')
const START_OF_LAST_RELEASE_PATTERN = /(^##? (?!Change Log$)|<a name=)/m
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that I really don't know of a bullet-proof way to do this because the format of where the "old content" begins is really dependent on the templates being used. For example, Angular may use either one # or two ## depending on what kind of version it is (patch, etc).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 44f6381 on jthomerson:fix-305-header-repeat into 04513e8 on conventional-changelog:master.

@ffxsam
Copy link

ffxsam commented Mar 1, 2019

This wasn't an issue in standard-version 4.x. I wonder what the code looks like there for making sure it doesn't keep writing the title over and over.

@jthomerson
Copy link
Contributor Author

jthomerson commented Mar 1, 2019

@ffxsam I didn't use the package at that version, so I'm not familiar with the headings it made, but it appears from looking at the code that the headings contained <a name=. The code was using oldContent.indexOf('<a name=') to find where the old content began.

Link to code:
https://github.com/conventional-changelog/standard-version/blob/v4.4.0/lib/lifecycles/changelog.js#L26-L28

@ffxsam
Copy link

ffxsam commented Mar 1, 2019

Wait, looks like this was fixed in 5.0.1. I just tested & confirmed.

See #292

@jthomerson
Copy link
Contributor Author

@ffxsam I also confirmed that the fix in 5.0.1 seems to be working. Of course, that change (b684c78) did not include any tests. So, you may still want to consider utilizing at least a portion of my PR here to add some tests so that you don't have a regression.

Also, both fixes have the same problem of not actually knowing how to parse a CHANGELOG since how you detect where the "old content" begins depends entirely on the format that's being used.

@bcoe
Copy link
Member

bcoe commented May 5, 2019

@jthomerson @ffxsam I'd honestly like to make the heading configurable (so that folks aren't completely tied to the default one); in the process, perhaps we could also make the regex that finds the last regex configurable -- tldr; I'd love to get this fixed, but maybe we should refactor the logic for putting the header in place a tiny bit.

@bcoe bcoe merged commit db2c6e5 into conventional-changelog:master May 5, 2019
@bcoe
Copy link
Member

bcoe commented May 5, 2019

@jthomerson thank you for the commit, I'm going to use this for a starting point and am going to make the header configurable 👍

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

Successfully merging this pull request may close these issues.

"# Change Log" title gets inserted on every release
4 participants