diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 109cd9fc57..92b771b1d5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -191,21 +191,46 @@ If the commit reverts a previous commit, it should begin with `revert: `, follow ### Type Must be one of the following: -* **feat**: A new feature -* **fix**: A bug fix -* **docs**: Documentation only changes -* **style**: Changes that do not affect the meaning of the code (white-space, formatting, missing - semi-colons, etc) -* **refactor**: A code change that neither fixes a bug nor adds a feature -* **perf**: A code change that improves performance -* **test**: Adding missing tests or correcting existing tests * **build**: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm) * **ci**: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs) -* **chore**: Other changes that don't modify `src` or `test` files +* **docs**: Documentation only changes +* **feat**: A new feature +* **fix**: A bug fix +* **perf**: A code change that improves performance +* **refactor**: A code change that neither fixes a bug nor adds a feature +* **style**: Changes that do not affect the meaning of the code (white-space, formatting, missing + semi-colons, etc) +* **test**: Adding missing tests or correcting existing tests ### Scope -The scope could be anything specifying place of the commit change. For example -`Compiler`, `ElementInjector`, etc. +The scope should be the name of the npm package affected (as perceived by person reading changelog generated from commit messages. + +The following is the list of supported scopes: + +* **common** +* **compiler** +* **compiler-cli** +* **core** +* **forms** +* **http** +* **language-service** +* **platform-browser** +* **platform-browser-dynamic** +* **platform-server** +* **platform-webworker** +* **platform-webworker-dynamic** +* **router** +* **upgrade** +* **tsc-wrapped** + +There is currently few exception to the "use package name" rule: + +* **packaging**: used for changes that change the npm package layout in all of our packages, e.g. public path changes, package.json changes done to all packages, d.ts file/format changes, changes to bundles, etc. +* **changelog**: used for updating the release notes in CHANGELOG.md +* none/empty string: useful for `style`, `test` and `refactor` changes that are done across all packages (e.g. `style: add missing semicolons`) + + +Packaging ### Subject The subject contains succinct description of the change: diff --git a/gulpfile.js b/gulpfile.js index ff2c446ae0..7ad5da28c5 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -126,7 +126,7 @@ gulp.task('public-api:update', ['build.sh'], (done) => { }); // Check the coding standards and programming errors -gulp.task('lint', ['format:enforce', 'tools:build'], () => { +gulp.task('lint', ['format:enforce', 'tools:build', 'validate-commit-messages'], () => { const tslint = require('gulp-tslint'); // Built-in rules are at // https://palantir.github.io/tslint/rules/ @@ -155,6 +155,40 @@ gulp.task('lint', ['format:enforce', 'tools:build'], () => { .pipe(tslint.report({emitError: true})); }); +gulp.task('validate-commit-messages', () => { + const validateCommitMessage = require('./tools/validate-commit-message'); + const childProcess = require('child_process'); + + // We need to fetch origin explicitly because it might be stale. + // I couldn't find a reliable way to do this without fetch. + childProcess.exec( + 'git fetch origin master && git log --reverse --format=%s HEAD ^origin/master', + (error, stdout, stderr) => { + if (error) { + console.log(stderr); + process.exit(1); + } + + let someCommitsInvalid = false; + let commitsByLine = stdout.trim().split(/\n/); + + console.log(`Examining ${commitsByLine.length} commits between HEAD and master`); + + if (commitsByLine.length == 0) { + console.log('There are zero new commits between this HEAD and master'); + } + + someCommitsInvalid = !commitsByLine.every(validateCommitMessage); + + if (someCommitsInvalid) { + console.log('Please fix the failing commit messages before continuing...'); + console.log( + 'Commit message guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines'); + process.exit(1); + } + }); +}); + gulp.task('tools:build', (done) => { tsc('tools/', done); }); // Check for circular dependency in the source code diff --git a/scripts/ci-lite/test_js.sh b/scripts/ci-lite/test_js.sh index 7f0c22b123..1c7bac81a0 100755 --- a/scripts/ci-lite/test_js.sh +++ b/scripts/ci-lite/test_js.sh @@ -20,6 +20,10 @@ echo 'travis_fold:start:test.unit.tools' # Run unit tests in tools node ./dist/tools/tsc-watch/ tools runCmdsOnly +cd tools/validate-commit-message +$(npm bin)/jasmine +cd - + echo 'travis_fold:end:test.unit.tools' diff --git a/tools/validate-commit-message/commit-message.json b/tools/validate-commit-message/commit-message.json new file mode 100644 index 0000000000..ec19bb7e13 --- /dev/null +++ b/tools/validate-commit-message/commit-message.json @@ -0,0 +1,35 @@ +{ + "maxLength": 100, + "types": [ + "build", + "ci", + "docs", + "feat", + "fix", + "perf", + "refactor", + "style", + "test" + ], + "scopes": [ + "benchpress", + "common", + "compiler", + "compiler-cli", + "core", + "forms", + "http", + "language-service", + "platform-browser", + "platform-browser-dynamic", + "platform-server", + "platform-webworker", + "platform-webworker-dynamic", + "router", + "upgrade", + "tsc-wrapped", + + "packaging", + "changelog" + ] +} diff --git a/tools/validate-commit-message/index.js b/tools/validate-commit-message/index.js new file mode 100644 index 0000000000..7bb7b84edc --- /dev/null +++ b/tools/validate-commit-message/index.js @@ -0,0 +1 @@ +module.exports = require('./validate-commit-message'); \ No newline at end of file diff --git a/tools/validate-commit-message/spec/support/jasmine.json b/tools/validate-commit-message/spec/support/jasmine.json new file mode 100644 index 0000000000..31a39bc6a2 --- /dev/null +++ b/tools/validate-commit-message/spec/support/jasmine.json @@ -0,0 +1,11 @@ +{ + "spec_dir": "", + "spec_files": [ + "**/*[sS]pec.js" + ], + "helpers": [ + "helpers/**/*.js" + ], + "stopSpecOnExpectationFailure": false, + "random": false +} diff --git a/tools/validate-commit-message/validate-commit-message.js b/tools/validate-commit-message/validate-commit-message.js new file mode 100644 index 0000000000..bdfc727bac --- /dev/null +++ b/tools/validate-commit-message/validate-commit-message.js @@ -0,0 +1,52 @@ +#!/usr/bin/env node + +/** + * GIT commit message format enforcement + * + * Note: this script was originally written by Vojta for AngularJS :-) + */ + +'use strict'; + +const fs = require('fs'); +const path = require('path'); +const configPath = path.resolve(__dirname, './commit-message.json'); +const config = JSON.parse(fs.readFileSync(configPath, 'utf8')); +const PATTERN = /^(revert\: )?(\w+)(?:\(([^)]+)\))?\: (.+)$/; + +module.exports = function(commitSubject) { + if (commitSubject.length > config['maxLength']) { + error(`The commit message is longer than ${config['maxLength']} characters`, commitSubject); + return false; + } + + const match = PATTERN.exec(commitSubject); + if (!match || match[2] === 'revert') { + error( + `The commit message does not match the format of "(): OR revert: type(): "`, + commitSubject); + return false; + } + + const type = match[2]; + if (config['types'].indexOf(type) === -1) { + error( + `${type} is not an allowed type.\n => TYPES: ${config['types'].join(', ')}`, commitSubject); + return false; + } + + const scope = match[3]; + + if (scope && !config['scopes'].includes(scope)) { + error( + `"${scope}" is not an allowed scope.\n => SCOPES: ${config['scopes'].join(', ')}`, + commitSubject); + return false; + } + + return true; +}; + +function error(errorMessage, commitMessage) { + console.error(`INVALID COMMIT MSG: "${commitMessage}"\n => ERROR: ${errorMessage}`); +} diff --git a/tools/validate-commit-message/validate-commit-message.spec.js b/tools/validate-commit-message/validate-commit-message.spec.js new file mode 100644 index 0000000000..bb78633904 --- /dev/null +++ b/tools/validate-commit-message/validate-commit-message.spec.js @@ -0,0 +1,119 @@ +'use strict'; + +describe('validate-commit-message.js', function() { + var validateMessage = require('./validate-commit-message'); + var errors = []; + var logs = []; + + var VALID = true; + var INVALID = false; + + beforeEach(function() { + errors.length = 0; + logs.length = 0; + + spyOn(console, 'error').and.callFake(function(msg) { + errors.push(msg.replace(/\x1B\[\d+m/g, '')); // uncolor + }); + + spyOn(console, 'log').and.callFake(function(msg) { + logs.push(msg.replace(/\x1B\[\d+m/g, '')); // uncolor + }); + }); + + describe('validateMessage', function() { + + it('should be valid', function() { + expect(validateMessage('fix(core): something')).toBe(VALID); + expect(validateMessage('feat(common): something')).toBe(VALID); + expect(validateMessage('docs(compiler): something')).toBe(VALID); + expect(validateMessage('style(http): something')).toBe(VALID); + expect(validateMessage('refactor(platform-webworker): something')).toBe(VALID); + expect(validateMessage('test(language-service): something')).toBe(VALID); + expect(validateMessage('test(packaging): something')).toBe(VALID); + expect(errors).toEqual([]); + }); + + + it('should fail when scope is invalid', function() { + expect(validateMessage('fix(Compiler): something')).toBe(INVALID); + expect(validateMessage('feat(bah): something')).toBe(INVALID); + expect(validateMessage('docs(animations): something')).toBe(INVALID); + expect(validateMessage('style(webworker): something')).toBe(INVALID); + expect(validateMessage('refactor(security): something')).toBe(INVALID); + expect(validateMessage('refactor(docs): something')).toBe(INVALID); + ['INVALID COMMIT MSG: "fix(Compiler): something"\n' + + ' => ERROR: "Compiler" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "feat(bah): something"\n' + + ' => ERROR: "bah" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "docs(animations): something"\n' + + ' => ERROR: "animations" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "style(webworker): something"\n' + + ' => ERROR: "webworker" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "refactor(security): something"\n' + + ' => ERROR: "security" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "refactor(docs): something"\n' + + ' => ERROR: "docs" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog'] + .forEach((expectedErrorMessage, index) => { + expect(expectedErrorMessage).toEqual(errors[index]); + }); + }); + + + it('should validate 100 characters length', function() { + var msg = + 'fix(compiler): something super mega extra giga tera long, maybe even longer and longer and longer... '; + + expect(validateMessage(msg)).toBe(INVALID); + expect(errors).toEqual([ + 'INVALID COMMIT MSG: "fix(compiler): something super mega extra giga tera long, maybe even longer and longer and longer... "\n => ERROR: The commit message is longer than 100 characters' + ]); + }); + + + it('should validate "(): " format', function() { + var msg = 'not correct format'; + + expect(validateMessage(msg)).toBe(INVALID); + expect(errors).toEqual([ + 'INVALID COMMIT MSG: "not correct format"\n => ERROR: The commit message does not match the format of "(): OR revert: type(): "' + ]); + }); + + + it('should support "revert: type(scope):" syntax and reject "revert(scope):" syntax', function() { + let correctMsg = 'revert: fix(compiler): reduce generated code payload size by 65%'; + expect(validateMessage(correctMsg)).toBe(VALID); + + let incorretMsg = 'revert(compiler): reduce generated code payload size by 65%'; + expect(validateMessage(incorretMsg)).toBe(INVALID); + expect(errors).toEqual([ + 'INVALID COMMIT MSG: "revert(compiler): reduce generated code payload size by 65%"\n => ERROR: The commit message does not match the format of "(): OR revert: type(): "' + ]); + }); + + + it('should validate type', function() { + expect(validateMessage('weird($filter): something')).toBe(INVALID); + expect(errors).toEqual( + ['INVALID COMMIT MSG: "weird($filter): something"\n' + + ' => ERROR: weird is not an allowed type.\n' + + ' => TYPES: build, ci, docs, feat, fix, perf, refactor, style, test']); + }); + + + it('should allow empty scope', + function() { expect(validateMessage('fix: blablabla')).toBe(VALID); }); + + // we don't want to allow WIP. it's ok to fail the PR build in this case to show that there is + // work still to be done. + it('should not ignore msg prefixed with "WIP: "', + function() { expect(validateMessage('WIP: bullshit')).toBe(INVALID); }); + }); +}); \ No newline at end of file