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

Empty body with breaking changes field present #2813

Closed
1 of 4 tasks
btd1337 opened this issue Oct 13, 2021 · 4 comments · Fixed by #2915
Closed
1 of 4 tasks

Empty body with breaking changes field present #2813

btd1337 opened this issue Oct 13, 2021 · 4 comments · Fixed by #2915
Labels

Comments

@btd1337
Copy link

btd1337 commented Oct 13, 2021

Expected Behavior

  • leave the body field empty.
  • check the breaking changes (y) option
  • inform the body after breakingBody message be displayed
  • body text be added to commit message

Current Behavior

  • body message not being added to commit message when it's inserted after breakingBody prompt

Affected packages

  • cli
  • core
  • prompt
  • config-angular

Possible Solution

Steps to Reproduce (for bugs)

  • git commit
  • select type
  • select scope
  • add short description
  • leave the body (long description) empty, press .
  • insert y when prompt ask if there are breaking changes
  • breakingBody message is displayed
  • insert body (long commit description)
  • follow to the end of the process
  • default commit message editor is opened to confirm the commit message
  • Note that the body field will be empty.
commitlint.config.js ```js ```

Context

Your Environment

Executable Version
commitlint --version 13.2.1
git --version 2.30.1 (Apple Git-130)
node --version 12.18.3
@escapedcat escapedcat added the bug label Oct 14, 2021
@honzamelena
Copy link
Contributor

I believe this particular row is the issue:

const commitBody = body ?? breakingBody ?? issuesBody ?? '-';

I think that skipping a body message actually puts an empty string to the variable and therefore the mentioned row will always evaluate as "not falsy (it is not null nor undefined)" for the body, hence skipping breakingBody and issuesBody. Simply changing it from "??" to "||" should solve this issue.

The following was taken from the official documentation of nullish coalescing operators. Please, check the last example as it shows this behavior in action.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator

Assigning a default value to a variable

Earlier, when one wanted to assign a default value to a variable, a common pattern was to use the logical OR operator (||):

let foo;

//  foo is never assigned any value so it is still undefined
let someDummyText = foo || 'Hello!';

However, due to || being a boolean logical operator, the left hand-side operand was coerced to a boolean for the evaluation and any falsy value (0, '', NaN, null, undefined) was not returned. This behavior may cause unexpected consequences if you consider 0, '', or NaN as valid values.

let count = 0;
let text = "";

let qty = count || 42;
let message = text || "hi!";
console.log(qty);     // 42 and not 0
console.log(message); // "hi!" and not ""

The nullish coalescing operator avoids this pitfall by only returning the second operand when the first one evaluates to either null or undefined (but no other falsy values):

let myText = ''; // An empty string (which is also a falsy value)

let notFalsyText = myText || 'Hello world';
console.log(notFalsyText); // Hello world

let preservingFalsy = myText ?? 'Hi neighborhood';
console.log(preservingFalsy); // '' (as myText is neither undefined nor null)

@escapedcat
Copy link
Member

Hey @honzamelena , thanks for the digging! Wanna give it a try and create PR for this?

@honzamelena
Copy link
Contributor

@escapedcat I'll try to have a look at it in the upcoming days.

@escapedcat
Copy link
Member

Will be in the next release

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

Successfully merging a pull request may close this issue.

3 participants