fix(writer): don't print category if empty #16
Conversation
lib/writer.js
Outdated
| @@ -67,7 +67,7 @@ exports.markdown = function (version, commits, options) { | |||
| Object.keys(this.types[type]).forEach(function (category) { | |||
| var prefix = '*'; | |||
| var nested = types[type][category].length > 1; | |||
| var categoryHeading = '* **' + category + ':**'; | |||
| var categoryHeading = prefix + (Boolean(category) ? ' **' + category + ':**' : ''); | |||
There was a problem hiding this comment.
I don't think you need to wrap category in Boolean. The empty string will still be false-y and go into the else case.
There was a problem hiding this comment.
Well... your lint setup told me to use Boolean(category) instead of !!category.
But you're right the empty category string is falsy enough ;)
lib/writer.js
Outdated
| @@ -67,7 +67,7 @@ exports.markdown = function (version, commits, options) { | |||
| Object.keys(this.types[type]).forEach(function (category) { | |||
| var prefix = '*'; | |||
| var nested = types[type][category].length > 1; | |||
| var categoryHeading = '* **' + category + ':**'; | |||
| var categoryHeading = prefix + (Boolean(category) ? ' **' + category + ':**' : ''); | |||
|
|
|||
| if (nested) { | |||
There was a problem hiding this comment.
A problem that arrises is when there are more than one category-less commits. It would then produce the following markdown:
*
* other changes
* more changeswhich then gets rendered as:
-
- other changes
- more changes
I think in this case, we don't want to nest them. So this if statement should be if (nested && category). That way, even if there are multiple commits with no category, prefix will still just be * and we don't have a separate line for the categoryHeading.
It would probably be good to write a test for this case too.
There was a problem hiding this comment.
Very good point! I've forgotten that an empty string can be still used as types[type][category]'s key 😆
test/writer.test.js
Outdated
| @@ -144,6 +144,28 @@ describe('writer', function () { | |||
| }); | |||
| }); | |||
|
|
|||
| it('ommits commit category if there was no category defined', function () { | |||
test/writer.test.js
Outdated
| var hash = '1234567'; | ||
| var commits = [ | ||
| { type: 'feat', category: 'testing', subject: 'did some testing', hash: hash + 'a' }, | ||
| { type: 'chore', category: '', subject: 'other changes', hash: hash + 'a' } |
There was a problem hiding this comment.
Is there a reason you're adding a to the end of the hash?
There was a problem hiding this comment.
No! I just didn't like the fact that two commits would eventually have the same hash...
test/writer.test.js
Outdated
| ]; | ||
|
|
||
| return Writer.markdown(VERSION, commits, {}) | ||
| .then(function (changelog) { |
There was a problem hiding this comment.
Like in the other PR, can you align this promise chain with the return?
test/writer.test.js
Outdated
| .then(function (line) { | ||
| var regex = new RegExp('^\\* other changes \\(1234567a\\)$'); | ||
|
|
||
| Expect(line).to.match(regex); |
There was a problem hiding this comment.
Since this regex is just checking that it equals a literal string, you can just use .to.eql instead:
Expect(line).to.eql('* other changes (1234567)');There was a problem hiding this comment.
I'm now checking both lines with Chai's .equal()
|
Just published v1.2.0! Thanks again! |
fixes #6 and #7