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.

Sign up for GitHub

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

Jump to bottom

feat: print commit message when the message is invalid #302

Merged
merged 1 commit into from
Mar 19, 2018
Merged

feat: print commit message when the message is invalid #302

merged 1 commit into from
Mar 19, 2018

Conversation

dnamorim
Copy link
Contributor

Print the full commit message when the input message is invalid.

Description

On @commitlint/cli when a commit message has validation errors, it prints the full commit message, returned by the lint function, otherwise, it prints just the commit header, just like before.

On @commitlint/lint, the return object returns the input parsed (useful for delete comments on editors like vim).

Motivation and Context

Currently, when using the CLI, if the message is invalid we lose all its content (using a custom message in VIM mode), which might be a little frustrating when we a have a long commit body, so it would be nice if we show the previous message after the linting errors.

Closes #222

Usage examples

For an invalid message

  1. git commit
  2. Input commit message:
foo

Lorem ipsum dolor sit amet consectetur adipisicing elit.

Output is:

⧗   input:
foo

Lorem ipsum dolor sit amet consectetur adipisicing elit.

✖   type may not be empty [type-empty]
✖   found 1 problems, 0 warnings

For a valid message

  1. git commit
  2. Input commit message:
feat: add foo

Lorem ipsum dolor sit amet consectetur adipisicing elit.

Output is:

⧗   input: feat: add foo
✖   found 0 problems, 0 warnings

How Has This Been Tested?

  • Added unit tests on @commitlint/lint ensuring that:

    • the lint object returns the input parsed when the commit message is just a header; header and body; header, body and footer.
    • the lint object returns the input if ignored

  • Added unit tests on @commitlint/cli to ensure the different type of outputs valid/invalid scenarios, with small or big commit messages.

  • Functional testing by using the commit command of the mono-repo.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

marionebl
marionebl reviewed Feb 28, 2018
View reviewed changes
Copy link
Contributor

@marionebl marionebl left a comment

Choose a reason for hiding this comment

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

This is great, thanks! Some comments, let me know what you think.

@commitlint/lint/src/index.js
@@ -4,13 +4,23 @@ import parse from '@commitlint/parse';
import implementations from '@commitlint/rules';
import entries from 'lodash.topairs';

const buildCommitMesage = ({header, body, footer}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand what this is intended to solve, could you drop some lines about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, so I've added this function so that on @commitlint/cli package it can print a parsed commit message (meaning, without comments or other parser configs). So, with this "helper", the string returned on the lint object contains the commit message (header, body, footer combined, if they are valid strings).

If you don't agree with this implementation or you see a better way of doing this, please let me know to see what I can improve 🙂

@commitlint/lint/src/index.test.js Outdated
@@ -31,10 +31,13 @@ test('negative on stub message and broken rule', async t => {
});

test('positive on ignored message and broken rule', async t => {
const actual = await lint('Revert "some bogus commit"', {
const message = 'Revert "some bogus commit"';
Copy link
Contributor

Choose a reason for hiding this comment

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

Those changes are unrelated, please revert them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes reverted.

@dnamorim dnamorim force-pushed the feat_print-commit-msg-error branch from 73171e9 to 6ac65d0 Compare March 5, 2018 09:28
feat: print commit message when the message is invalid
5a75986 
When using the CLI, if the message is invalid we lose all its content (using a custom message in VIM mode), which might be a little frustrating when we a have a long commit body.

This feature prints the full message when there are linting errors.

Closes conventional-changelog#222
@dnamorim dnamorim force-pushed the feat_print-commit-msg-error branch from 6ac65d0 to 5a75986 Compare March 5, 2018 11:27
@eMerzh
Copy link

eMerzh commented Mar 5, 2018

what do you guys think about also adding a small "header" so that if it's integrated in the CI or in a hook, it display smth like :
"The commit message is invalid" that might help understand why the thing is failing

brunops
brunops reviewed Mar 7, 2018
View reviewed changes
@commitlint/cli/src/cli.js
const input =
report.errors.length > 0
? `\n${report.input}\n`
: message.split('\n')[0];
Copy link

@brunops brunops Mar 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why not print the entire input always? Seems good info even when the commit is successful...

const input = `\n${report.input}\n`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On #222, I have proposed to only print the full message when an error occurs, because if your input passes on commitlint, you can always amend it but, if the input validation fails, then you lost all the commit text, making sense to print the message. This way, you can copy the message and fix the highlighted issues in it, before making a new commit.

However, if more people agree to show the full input, I can change it. @marionebl, what are your thoughts about this?

Copy link

Choose a reason for hiding this comment

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

@dnamorim totally agree that printing the entire message on error is definitely a must, for the exact reason you've mentioned. Just mentioning that printing the entire message on success can be nice too, @marionebl thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's stay with the printing input only on error, but revisit printing it always later.

@marionebl marionebl merged commit 86c34f1 into conventional-changelog:master Mar 19, 2018
@marionebl
Copy link
Contributor

Thanks, will publish a release tomorrow.

@dnamorim dnamorim deleted the feat_print-commit-msg-error branch March 19, 2018 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marionebl marionebl marionebl left review comments

@brunops brunops brunops left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Print commit message after error message if it is invalid
4 participants
@dnamorim @eMerzh @marionebl @brunops