From f1b9b2cbbfc78380badcbf5874feaba031f0cf93 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 29 Nov 2016 20:18:41 +0100 Subject: [PATCH] chore(forbidden-identifiers): detect incorrect line endings (#1722) * chore(forbidden-identifiers): detect incorrect line endings * Removes stale scope checking (from `@angular2-material/$COMPONENT` times) * Adds support for error grouping (useful when having a file full of invalid line endings) * Moves chains into exta functions to understand procession easily. * Update comment wording --- scripts/ci/forbidden-identifiers.js | 215 ++++++++++++---------------- 1 file changed, 92 insertions(+), 123 deletions(-) diff --git a/scripts/ci/forbidden-identifiers.js b/scripts/ci/forbidden-identifiers.js index 26dbe0d48d0..301f2224602 100755 --- a/scripts/ci/forbidden-identifiers.js +++ b/scripts/ci/forbidden-identifiers.js @@ -26,12 +26,11 @@ const blocked_statements = [ '\\bxit\\(', '\\bdebugger;', 'from \\\'rxjs/Rx\\\'', + '\\r' ]; const sourceFolders = ['./src', './e2e']; -const libRoot = 'src/lib'; const blockedRegex = new RegExp(blocked_statements.join('|'), 'g'); -const importRegex = /from\s+'(.*)';/g; /* * Verify that the current PR is not adding any forbidden identifiers. @@ -47,69 +46,105 @@ findTestFiles() .then(files => files.map(name => ({ name, content: fs.readFileSync(name, 'utf-8') }))) /* Run checks against content of the filtered files. */ - .then(diffList => { + .then(diffList => findErrors(diffList)) - return diffList.reduce((errors, diffFile) => { - let fileName = diffFile.name; - let content = diffFile.content.split('\n'); - let lineCount = 0; - - content.forEach(line => { - lineCount++; - - let matches = line.match(blockedRegex); - let scopeImport = path.relative(libRoot, fileName).startsWith('..') - ? isRelativeScopeImport(fileName, line) - : false; - - if (matches || scopeImport) { - - let error = { - fileName: fileName, - lineNumber: lineCount, - statement: matches && matches[0] || scopeImport.invalidStatement - }; - - if (scopeImport) { - error.messages = [ - 'You are using an import statement, which imports a file from another scope package.', - `Please import the file by using the following path: ${scopeImport.scopeImportPath}` - ]; - } - - errors.push(error); - } - }); - - return errors; - - }, []); - }) + /* Groups similar errors to simplify console output */ + .then(errors => groupErrors(errors)) /* Print the resolved errors to the console */ - .then(errors => { - if (errors.length > 0) { - console.error('Error: You are using one or more blocked statements:\n'); - - errors.forEach(entry => { - if (entry.messages) { - entry.messages.forEach(message => console.error(` ${message}`)) - } - - console.error(` ${entry.fileName}@${entry.lineNumber}, Statement: ${entry.statement}.\n`); - }); - - process.exit(1); - } - }) + .then(errors => printErrors(errors)) .catch(err => { // An error occurred in this script. Output the error and the stack. - console.error('An error occurred during execution:'); console.error(err.stack || err); process.exit(2); }); +/** + * Finds errors inside of changed files by running them against the blocked statements. + * @param {{name: string, content: string}[]} diffList + */ +function findErrors(diffList) { + return diffList.reduce((errors, diffFile) => { + let fileName = diffFile.name; + let content = diffFile.content.split('\n'); + let lineNumber = 0; + + content.forEach(line => { + lineNumber++; + + let matches = line.match(blockedRegex); + + if (matches) { + + errors.push({ + fileName, + lineNumber, + statement: matches[0] + }); + + } + }); + + return errors; + + }, []); +} + +/** + * Groups similar errors in the same file which are present over a range of lines. + * @param {{fileName: string, lineNumber: number, statement: string}[]} errors + */ +function groupErrors(errors) { + + let initialError, initialLine, previousLine; + + return errors.filter(error => { + + if (initialError && isSimilarError(error)) { + previousLine = error.lineNumber; + + // Overwrite the lineNumber with a string, which indicates the range of lines. + initialError.lineNumber = `${initialLine}-${previousLine}`; + + return false; + } + + initialLine = previousLine = error.lineNumber; + initialError = error; + + return true; + }); + + /** Checks whether the given error is similar to the previous one. */ + function isSimilarError(error) { + return initialError.fileName === error.fileName && + initialError.statement === error.statement && + previousLine === (error.lineNumber - 1); + } +} + +/** + * Prints all errors to the console and terminates the process if errors were present. + * @param {{fileName: string, lineNumber: number, statement: string}[]} errors + */ +function printErrors(errors) { + if (errors.length > 0) { + + console.error('Error: You are using one or more blocked statements:\n'); + + errors.forEach(entry => { + + // Stringify the statement to represent line-endings or other unescaped characters. + let statement = JSON.stringify(entry.statement); + + console.error(` ${entry.fileName}@${entry.lineNumber}, Statement: ${statement}.\n`); + }); + + // Exit the process with an error exit code to notify the CI. + process.exit(1); + } +} /** * Resolves all files, which should run against the forbidden identifiers check. @@ -120,8 +155,8 @@ function findTestFiles() { return findChangedFiles(); } - var files = sourceFolders.map(folder => { - return glob(`${folder}/**/*.ts`); + let files = sourceFolders.map(folder => { + return glob(`${folder}/**/*`); }).reduce((files, fileSet) => files.concat(fileSet), []); return Promise.resolve(files); @@ -150,72 +185,6 @@ function findChangedFiles() { }); } -/** - * Checks the line for any relative imports of a scope package, which should be imported by using - * the scope package name instead of the relative path. - * @param fileName Filename to validate the path - * @param line Line to be checked. - */ -function isRelativeScopeImport(fileName, line) { - if (fileName.startsWith(libRoot)) { - return false; - } - - let importMatch = importRegex.exec(line); - - // We have to reset the last index of the import regex, otherwise we - // would have incorrect matches in the next execution. - importRegex.lastIndex = 0; - - // Skip the check if the current line doesn't match any imports. - if (!importMatch) { - return false; - } - - let importPath = importMatch[1]; - - // Skip the check when the import doesn't start with a dot, because the line - // isn't importing any file relatively. Also applies to scope packages starting - // with `@`. - if (!importPath.startsWith('.')) { - return false; - } - - // Transform the relative import path into an absolute path. - importPath = path.resolve(path.dirname(fileName), importPath); - - let fileScope = findScope(fileName); - let importScope = findScope(importPath); - - if (fileScope.path !== importScope.path) { - - // Creates a valid import statement, which uses the correct scope package. - let validImportPath = `@angular/material`; - - return { - fileScope: fileScope.name, - importScope: importScope.name, - invalidStatement: importMatch[0], - scopeImportPath: validImportPath - } - } - - function findScope(filePath) { - // Normalize the filePath, to avoid issues with the different environments path delimiter. - filePath = path.normalize(filePath); - - // Iterate through all scope paths and try to find them inside of the file path. - var scopePath = filePath.indexOf(path.normalize(libRoot)) == -1 ? libRoot : filePath; - - // Return an object containing the name of the scope and the associated path. - return { - name: path.basename(scopePath), - path: scopePath - } - } - -} - /** * Executes a process command and wraps it inside of a promise. * @returns {Promise.}