fix(cli): exit with an error when validation fail even if there is no Git directory

[cmalard]

commitErrorLogPath can be undefined only when there is no Git directory There is a default in this last case :

commitErrorLogPath = gitDirectory + '/logs/incorrect-commit-msgs';

+ refactor cli I changed the style for declaring functions to be consistent with other files.

[codecov-io]

Codecov Report

Merging #108 into master will not change coverage. The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #108   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines         134    134           
=====================================
  Hits          134    134

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2e048fb...82a1dc1. Read the comment docs.

[Garbee]

I'm going to cherry pick out the bug fix and merge that. Please resubmit the refactor as its own PR. One PR should address one issue. That way everything is straight in the future finding where and how things happened.

The bug fix commit LGTM. Nice, clear, and addresses the problem with minimal changes.

[cmalard]

Hey Garbee,

I'm going to do that but why not using rebase merging ? This is the idea of Angular commit convention: enforce atomic commits. So each one has its own description (like here) and that way everything is straight in the future finding where and how things happened :-)

The refactor is partly motivated by the fix, that would simplify the contribution process.

[Garbee]

The PR here is for fixing a bug, not refactoring. I'm only going to pull the bug fix in. Please make a new PR and detail the refactor so it can be analyzed more accurately.

I know I can rebase the merge just fine. I prefer that. However, I am focusing on the discussion within the PR and the details it provides to the commits. Your refactor is touching a lot more lines than the bug fix. So, let's have a new PR with details on what exactly that commit is doing.

One PR = One thing to address. This is one PR trying to do two different things. If you have a handful of commits from working on something that all go to the same PR that's fine. They can be squashed and merged in. This however, is two commit doing two different things and only one addresses the details provided in the PR.

For me, seeing someone submit a bug fix then tack on "oh also some refactoring" with no more/very little detail is pretty scary. I'm not sure how to review what is going on with those changes. Each thing has its own merit to be discussed and reviewed. I'd like us to have a clear discussion history on things beyond simply the commit history.

[cmalard]

Well, so this was more a problem with the naming of the first commit ?

109 created, we can discuss the refactor there.

It means this PR is waiting #109 to move on.

[Garbee]

It isn't as much a problem with naming as much as one PR trying to do two things. I don't like that.

Cherry picking this one commit out is difficult since it is stacked on the old one. I'm not sure exactly what the changes to the lower end of the file need to be. So I'm closing this PR out completely.

We can discuss #109 and move from there to get the bug fix in.

I really do appreciate the thought at trying to make things more consistent. But, one PR needs to do one thing. Not multiple things. If it can't easily be squashed into a single commit that signifies what is going on, it isn't a good PR.