feat: add support for breaking changes #34

JasonCust · 2017-12-15T06:06:04Z

Basing this PR off of feedback for a previous PR #21 with the modification that it would use a more generic and extensible flag option in the commit subject.

Should resolve request #20

robinjoseph08

Thanks @JasonCust for carrying the previous PR! I left a few comments and suggestions, but if you have any questions, feel free to ask!

robinjoseph08 · 2017-12-16T21:59:34Z

lib/git.js

@@ -4,7 +4,7 @@ var Bluebird = require('bluebird');
 var CP       = Bluebird.promisifyAll(require('child_process'));

 var SEPARATOR      = '===END===';
-var COMMIT_PATTERN = /^(\w*)(\(([\w\$\.\-\* ]*)\))?\: (.*)$/;
+var COMMIT_PATTERN = /^(\w*)(?:\(([^)]*?)\)|):\s*(.*?(?:\[([^\]]+?)\]|))$/;


Can you add a \s* right before the $ at the end? That way, it will also parse the flags correctly even if there's a trailing space on the commit message.

I can make this even a bit more flexible by allowing the flags to occur anywhere in the subject line.

I think it might be better to enforce it only at the end. I'd rather it be well-defined on when it'll be picked up as opposed to unintentionally being picked up.

robinjoseph08 · 2017-12-16T22:00:28Z

lib/git.js

+    commit.subject = parsed[3];
+
+    if (parsed[4]) {
+      parsed[4].toLowerCase().split(/\s*,\s*/).forEach(function (flag) {


Can you add a .trim() right after the .toLowerCase() so account for [ breaking ]?

I'll move inside the forEach to apply the trim to every flag.

robinjoseph08 · 2017-12-16T22:06:18Z

lib/git.js

+    if (parsed[4]) {
+      parsed[4].toLowerCase().split(/\s*,\s*/).forEach(function (flag) {
+        switch (flag) {
+          case 'breaking': commit.type = flag;


I know it doesn't matter here too much since it's just one case and it's a one-liner, but can you format it like so:

switch (flag) { case 'breaking': commit.type = flag; break; }

robinjoseph08 · 2017-12-16T22:07:38Z

test/git.test.js

        Expect(commits[0]).to.have.property('type');
        Expect(commits[0]).to.have.property('category');
        Expect(commits[0]).to.have.property('subject');
+
+        Expect(commits[5].type).to.eql('breaking');


As opposed to including this check on an existing test, can you add an explicit test for the breaking change logic?

robinjoseph08 · 2017-12-16T22:08:53Z

lib/writer.js

@@ -6,6 +6,7 @@ var DEFAULT_TYPE = 'other';
 var PR_REGEX = new RegExp(/#[1-9][\d]*/g);
 var TYPES = {
  build: 'Build System / Dependencies',
+  breaking: 'Breaking Changes',


It goes the alphabetical sorting of the others, but I think that breaking changes should probably be at the top of the changelog so that they are more highlighted.

robinjoseph08 · 2017-12-16T22:12:34Z

README.md

@@ -40,6 +40,10 @@ Where `type` is one of the following:
 * `style`
 * `test`

+Where `flags` is one or more of the following (comma separated):


Nit: Change it to:

Where `flags` is an optional comma-separated list of one or more of the following (must be surrounded in square brackets):

to make it clear that they're optional and that they need to be surrounded by []s.

robinjoseph08 · 2017-12-16T22:12:55Z

README.md

@@ -22,7 +22,7 @@ $ npm i generate-changelog -g # install it globally
 To use this module, your commit messages have to be in this format:

 ```
-type(category): description
+type(category): description [flags]
 ```

 Where `type` is one of the following:


Since we're also adding breaking as a type, can you add it to this list as well?

…rimmed

JasonCust · 2017-12-17T03:36:23Z

Changes should be all set to go.

robinjoseph08 · 2017-12-17T04:13:44Z

lib/git.js

+      parsed[4].toLowerCase().split(',').forEach(function (flag) {
+        flag = flag.trim();
+
+        switch (flag) {


Thoughts on just making this flag.trim() instead of reassigning flag?

The trimmed version needs to be captured either via reassignment or creating a new variable since it used in more than one place (line 77 assigns the value to type in the case of a breaking flag). As far as reassignment goes, in this case I don't see as an issue in the same way reassigning a function parameter for a module API would be for two reasons:

This is essentially a pipeline that is parsing and formatting the flags prior to processing the logic for each flag. I could refactor the trim to occur in a map prior to the switch but I didn't see the need to loop the split flag array twice at this time.

Since flag is a primitive value, reassigning it does not have any side effects (as modifying an object property would).

The trimmed version needs to be captured either via reassignment or creating a new variable since it used in more than one place (line 77 assigns the value to type in the case of a breaking flag). As far as reassignment goes, in this case I don't see it as an issue in the same manner as reassigning a function parameter for a module API would be for two reasons:

This is essentially a pipeline that is parsing and formatting the flags prior to processing the logic for each flag. I could refactor the trim to occur in a map prior to the switch but I didn't see the need to loop the split flag array twice at this time.

Since flag is a primitive value, reassigning it does not have any side effects (as modifying an object property would).

robinjoseph08

Last 2 things and it'll be ready to be merged!

robinjoseph08

LGTM! Thanks again for the contribution! I'll let you know when a new version has been published.

robinjoseph08 · 2017-12-17T19:09:48Z

Version v1.7.0 has been published which includes this change. Thanks again for your help!

JasonCust added 5 commits December 15, 2017 00:58

test(valid-commits): add commit message for breaking change

29b9f69

test(git): ensure breaking flag sets type correctly and can be filtered

8eb1271

feat(writer): add new type for breaking changes

7e7aae4

feat(git): switch type to breaking if flag is set in commit subject

889659b

docs(README): include documentation for optional flags

d6a16e8

JasonCust changed the title ~~feat: add support for breaking changes (#12)~~ feat: add support for breaking changes Dec 15, 2017

JasonCust mentioned this pull request Dec 15, 2017

Breaking change support #20

Closed

robinjoseph08 self-assigned this Dec 15, 2017

robinjoseph08 suggested changes Dec 16, 2017

View reviewed changes

robinjoseph08 assigned JasonCust and unassigned robinjoseph08 Dec 16, 2017

JasonCust added 4 commits December 16, 2017 22:25

docs(README): include breaking as a type

dbf1acc

test(git): ensure flags are parsed from anywhere in the subject and t…

655494c

…rimmed

test(git): parse flags from anywhere in the subject and trim them

2765661

refactor(writer): reorder types

188f0ab

robinjoseph08 reviewed Dec 17, 2017

View reviewed changes

JasonCust added 2 commits December 17, 2017 08:13

test(git): ensure flags can only occur that the end of a subject

7c71a27

fix(git): only parse flags that occur at the end of a subject

f2369b9

robinjoseph08 approved these changes Dec 17, 2017

View reviewed changes

robinjoseph08 merged commit d697b27 into lob:master Dec 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for breaking changes #34

feat: add support for breaking changes #34

JasonCust commented Dec 15, 2017 •

edited

Loading

robinjoseph08 left a comment

robinjoseph08 Dec 16, 2017

JasonCust Dec 17, 2017

robinjoseph08 Dec 17, 2017

JasonCust Dec 17, 2017

robinjoseph08 Dec 16, 2017

JasonCust Dec 17, 2017

robinjoseph08 Dec 16, 2017

JasonCust Dec 17, 2017

robinjoseph08 Dec 16, 2017

JasonCust Dec 17, 2017

robinjoseph08 Dec 16, 2017

JasonCust Dec 17, 2017

robinjoseph08 Dec 16, 2017

JasonCust Dec 17, 2017

robinjoseph08 Dec 16, 2017

JasonCust Dec 17, 2017

JasonCust commented Dec 17, 2017

robinjoseph08 Dec 17, 2017

JasonCust Dec 17, 2017

JasonCust Dec 17, 2017 •

edited

Loading

robinjoseph08 left a comment

robinjoseph08 left a comment

robinjoseph08 commented Dec 17, 2017

feat: add support for breaking changes #34

feat: add support for breaking changes #34

Conversation

JasonCust commented Dec 15, 2017 • edited Loading

robinjoseph08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonCust commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonCust Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

robinjoseph08 left a comment

Choose a reason for hiding this comment

robinjoseph08 left a comment

Choose a reason for hiding this comment

robinjoseph08 commented Dec 17, 2017

JasonCust commented Dec 15, 2017 •

edited

Loading

JasonCust Dec 17, 2017 •

edited

Loading