1
\$\begingroup\$

I wrote a commit message validator + CLI in NodeJS.

Notes:

  • My greatest concern is that the patten I use to represent the results of a validation is inadequate. Options:

    • Use adhoc enums
    • Create a ValidationResult class (?)
    • Extend Error and return an array of those. In this case, how would the --verbose option, which shows passed rules and informational rules work?
  • Is the pattern I use in lib/rules.js to represent each of the rules adequate?

  • Is there a better (performance and/or readability-wise) way of stripping commit messages than the approach I use in lib/strip.js?

  • Other feedback is welcome too, of course.

index.js:

const strip = require('./lib/strip');
const rules = require('./lib/rules');

function validate(commitMessage) {
    const strippedMessage = strip(commitMessage);
    const results = [];

    for (const rule of rules) {
        let outcome;

        if (rule.test === undefined) {
            outcome = 'info';

            results.push({
                type: outcome,
                message: rule.message
            });

            continue;
        }

        if (rule.test(strippedMessage)) {
            outcome = 'pass';
        } else {
            outcome = 'fail';
        }

        results.push({
            type: outcome,
            message: rule.message
        });
    }

    return results;
}

module.exports = validate;

cli.js:

#!/usr/bin/env node

const meow = require('meow');
const fs = require('fs');
const stdin = require('get-stdin');

const validate = require('.');

const log = require('./lib/log');

const cli = meow({
    description: 'Validate commit messages against the seven rules of commit messages.',

    flags: {
        silent: {
            type: 'boolean',
            alias: 's'
        },

        verbose: {
            type: 'boolean',
            alias: 'v'
        },

        // TODO: Work with --file flag
        file: {
            type: 'string',
            alias: 'f'
        }

        // TODO: If nothing else, read stdin
    }
});

log(); // Leading new line


if (cli.input.length === 0) {
    // TODO: Cry
}

const [commitMessage] = cli.input;

const results = validate(commitMessage);
let exitCode = 0;

for (const result of results) {
    switch (result.type) {
    case 'fail':
        if (!cli.flags.silent) {
            log.error(result.message);
        }

        if (exitCode === 0) { exitCode = 1; }

        break;
    case 'pass':
    case 'info':
        if (!cli.flags.silent && cli.flags.verbose) {
            log[result.type](result.message);
        }

        break;
    default:
        throw new Error(`Internal Error: Invalid result type '${result.type}'`);
    }
}

process.exit(exitCode);

lib/

lib/rules.js:

const one = {
    message: 'Separate subject from body with a blank line',
    test: (commitMessage) => {
        const separatedByLf = /^.+(\n\n(?:.|\n)+|\n?)$/g;

        return separatedByLf.test(commitMessage);
    }
};

const two = {
    message: 'Limit the subject line to 50 characters',
    test: (commitMessage) => {
        const subjectLine = getSubjectLine(commitMessage);
        const cutOff = 50;

        return subjectLine.length < cutOff;
    }
};

const three = {
    message: 'Capitalize the subject line',
    test: (commitMessage) => {
        const subjectLine = getSubjectLine(commitMessage);
        const firstCharacter = subjectLine[0];

        return !isLowerCase(firstCharacter);
    }
};

const four = {
    message: 'Do not end the subject line with a period',
    test: (commitMessage) => {
        const subjectLine = getSubjectLine(commitMessage);
        const lastCharacter = subjectLine.substr(-1);

        return !(lastCharacter === '.');
    }
};

const five = {
    message: 'Use the imperative mood in the subject line'
    // We could, in theory, use NLP to check for this rule,
    // ...but it would take effort and would be error prone
};

const six = {
    message: 'Wrap the body at 72 characters',
    test: (commitMessage) => {
        const bodyLines = getBody(commitMessage).split('\n');

        return bodyLines.every(line => line.length < 72);
    }
};

const seven = {
    message: 'Use the body to explain _what_ and _why_ vs. _how_'
    // This is obviously not detectable programtically
};

const rules = [
    one, two, three, four, five, six, seven
];

module.exports = rules;

function getSubjectLine(commitMessage) {
    return commitMessage.split('\n')[0];
}

function getBody(commitMessage) {
    const [, ...body] = commitMessage.split('\n');
    return body.join('\n');
}

function isLowerCase(char) {
    return !(char.toUpperCase() === char);
}

lib/log.js:

const chalk = require('chalk');
const logSymbols = require('log-symbols');

function log(...args) {
    args = args.join(' ');
    console.log(args);
}

log.error = (...args) => {
    console.error(chalk.red(logSymbols.error, args));
};

log.success = (...args) => {
    log(chalk.green(logSymbols.success, args));
};

log.warn = (...args) => {
    console.warn(chalk.yellow(logSymbols.warn, args));
};

log.info = (...args) => {
    console.info(chalk.blue(logSymbols.info, args));
};

module.exports = log;

lib/strip.js:

// See: https://git-scm.com/docs/git-commit#git-commit-strip

// TODO: Respect git's core.commentchar

function strip(rawCommitMessage) {
    const trailingWhitespace = /[ \t\f\v]+$/gm;
    const commentary = /^#.*/gm;
    const consecutiveEmptyLines = /\n{3,}/g;
    const leadingTrailingEmptyLines = /^\n+|\n+$/g;

    return rawCommitMessage
        .replace(trailingWhitespace, '')
        .replace(commentary, '')
        .replace(consecutiveEmptyLines, '\n\n')
        .replace(leadingTrailingEmptyLines, '');
}

module.exports = strip;

\$\endgroup\$
1
\$\begingroup\$

Your code looks mostly good, but here are some things:

  • lib/log.js: You don't need the log function. console.log does exactly what you do in that function (well not exactly, but it shouldn't make a difference in this context). You can just set log = (...args) => console.log(...args) or function log(...args) {console.log(...args)}
  • lib/rules.js In the isLowerCase function, you are checking !(char.toUpperCase() === char). You should use !==
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.