From abec0ee58357e83686758a44614f451783364c5b Mon Sep 17 00:00:00 2001 From: Krishna Acondy Date: Wed, 7 Apr 2021 11:05:26 +0100 Subject: [PATCH] fix(*): improve code and message texts, fix comments, add missing tests --- src/rules/hasMacroNameInMend.spec.ts | 56 ++++++++++++++++++++++----- src/rules/hasMacroNameInMend.ts | 50 +++++++++++++----------- src/rules/hasMacroParentheses.spec.ts | 14 +++++++ src/rules/hasMacroParentheses.ts | 28 +++++++------- src/rules/noNestedMacros.spec.ts | 10 ++--- src/rules/noNestedMacros.ts | 36 +++++++++-------- 6 files changed, 127 insertions(+), 67 deletions(-) diff --git a/src/rules/hasMacroNameInMend.spec.ts b/src/rules/hasMacroNameInMend.spec.ts index 5876305..f493b1c 100644 --- a/src/rules/hasMacroNameInMend.spec.ts +++ b/src/rules/hasMacroNameInMend.spec.ts @@ -28,7 +28,7 @@ describe('hasMacroNameInMend', () => { expect(hasMacroNameInMend.test(content)).toEqual([ { - message: '%mend missing macro name', + message: '%mend statement is missing macro name', lineNumber: 4, startColumnNumber: 3, endColumnNumber: 9, @@ -37,6 +37,44 @@ describe('hasMacroNameInMend', () => { ]) }) + it('should return an array with a single diagnostic when a macro is missing an %mend statement', () => { + const content = `%macro somemacro; + %put &sysmacroname;` + + expect(hasMacroNameInMend.test(content)).toEqual([ + { + message: 'Missing %mend statement for macro - somemacro', + lineNumber: 1, + startColumnNumber: 1, + endColumnNumber: 1, + severity: Severity.Warning + } + ]) + }) + + it('should return an array with a diagnostic for each macro missing an %mend statement', () => { + const content = `%macro somemacro; + %put &sysmacroname; + %macro othermacro` + + expect(hasMacroNameInMend.test(content)).toEqual([ + { + message: 'Missing %mend statement for macro - somemacro', + lineNumber: 1, + startColumnNumber: 1, + endColumnNumber: 1, + severity: Severity.Warning + }, + { + message: 'Missing %mend statement for macro - othermacro', + lineNumber: 3, + startColumnNumber: 1, + endColumnNumber: 1, + severity: Severity.Warning + } + ]) + }) + it('should return an array with a single diagnostic when %mend has incorrect macro name', () => { const content = ` %macro somemacro; @@ -45,7 +83,7 @@ describe('hasMacroNameInMend', () => { expect(hasMacroNameInMend.test(content)).toEqual([ { - message: 'mismatch macro name in %mend statement', + message: '%mend statement has mismatched macro name', lineNumber: 4, startColumnNumber: 9, endColumnNumber: 24, @@ -88,7 +126,7 @@ describe('hasMacroNameInMend', () => { expect(hasMacroNameInMend.test(content)).toEqual([ { - message: '%mend missing macro name', + message: '%mend statement is missing macro name', lineNumber: 6, startColumnNumber: 5, endColumnNumber: 11, @@ -110,7 +148,7 @@ describe('hasMacroNameInMend', () => { expect(hasMacroNameInMend.test(content)).toEqual([ { - message: '%mend missing macro name', + message: '%mend statement is missing macro name', lineNumber: 9, startColumnNumber: 3, endColumnNumber: 9, @@ -132,14 +170,14 @@ describe('hasMacroNameInMend', () => { expect(hasMacroNameInMend.test(content)).toEqual([ { - message: '%mend missing macro name', + message: '%mend statement is missing macro name', lineNumber: 6, startColumnNumber: 5, endColumnNumber: 11, severity: Severity.Warning }, { - message: '%mend missing macro name', + message: '%mend statement is missing macro name', lineNumber: 9, startColumnNumber: 3, endColumnNumber: 9, @@ -197,7 +235,7 @@ describe('hasMacroNameInMend', () => { expect(hasMacroNameInMend.test(content)).toEqual([ { - message: '%mend missing macro name', + message: '%mend statement is missing macro name', lineNumber: 29, startColumnNumber: 5, endColumnNumber: 11, @@ -216,7 +254,7 @@ describe('hasMacroNameInMend', () => { expect(hasMacroNameInMend.test(content)).toEqual([ { - message: 'mismatch macro name in %mend statement', + message: '%mend statement has mismatched macro name', lineNumber: 6, startColumnNumber: 14, endColumnNumber: 29, @@ -233,7 +271,7 @@ describe('hasMacroNameInMend', () => { expect(hasMacroNameInMend.test(content)).toEqual([ { - message: '%mend missing macro name', + message: '%mend statement is missing macro name', lineNumber: 4, startColumnNumber: 5, endColumnNumber: 11, diff --git a/src/rules/hasMacroNameInMend.ts b/src/rules/hasMacroNameInMend.ts index 2081f77..5a123a0 100644 --- a/src/rules/hasMacroNameInMend.ts +++ b/src/rules/hasMacroNameInMend.ts @@ -4,66 +4,72 @@ import { LintRuleType } from '../types/LintRuleType' import { Severity } from '../types/Severity' import { trimComments } from '../utils/trimComments' import { getLineNumber } from '../utils/getLineNumber' -import { getColNumber } from '../utils/getColNumber' +import { getColumnNumber } from '../utils/getColumnNumber' const name = 'hasMacroNameInMend' -const description = 'The %mend statement should contain the macro name' -const message = '$mend statement missing or incorrect' +const description = + 'Enforces the presence of the macro name in each %mend statement.' +const message = '%mend statement has missing or incorrect macro name' const test = (value: string) => { const diagnostics: Diagnostic[] = [] const statements: string[] = value ? value.split(';') : [] - const stack: string[] = [] - let trimmedStatement = '', - commentStarted = false + const declaredMacros: { name: string; lineNumber: number }[] = [] + let isCommentStarted = false statements.forEach((statement, index) => { - ;({ statement: trimmedStatement, commentStarted } = trimComments( + const { statement: trimmedStatement, commentStarted } = trimComments( statement, - commentStarted - )) + isCommentStarted + ) + isCommentStarted = commentStarted if (trimmedStatement.startsWith('%macro ')) { const macroName = trimmedStatement .slice(7, trimmedStatement.length) .trim() .split('(')[0] - stack.push(macroName) + declaredMacros.push({ + name: macroName, + lineNumber: getLineNumber(statements, index + 1) + }) } else if (trimmedStatement.startsWith('%mend')) { - const macroStarted = stack.pop() + const declaredMacro = declaredMacros.pop() const macroName = trimmedStatement .split(' ') .filter((s: string) => !!s)[1] if (!macroName) { diagnostics.push({ - message: '%mend missing macro name', + message: '%mend statement is missing macro name', lineNumber: getLineNumber(statements, index + 1), - startColumnNumber: getColNumber(statement, '%mend'), - endColumnNumber: getColNumber(statement, '%mend') + 6, + startColumnNumber: getColumnNumber(statement, '%mend'), + endColumnNumber: getColumnNumber(statement, '%mend') + 6, severity: Severity.Warning }) - } else if (macroName !== macroStarted) { + } else if (macroName !== declaredMacro!.name) { diagnostics.push({ - message: 'mismatch macro name in %mend statement', + message: '%mend statement has mismatched macro name', lineNumber: getLineNumber(statements, index + 1), - startColumnNumber: getColNumber(statement, macroName), + startColumnNumber: getColumnNumber(statement, macroName), endColumnNumber: - getColNumber(statement, macroName) + macroName.length - 1, + getColumnNumber(statement, macroName) + macroName.length - 1, severity: Severity.Warning }) } } }) - if (stack.length) { + + declaredMacros.forEach((declaredMacro) => { diagnostics.push({ - message: 'missing %mend statement for macro(s)', - lineNumber: statements.length + 1, + message: `Missing %mend statement for macro - ${declaredMacro.name}`, + lineNumber: declaredMacro.lineNumber, startColumnNumber: 1, endColumnNumber: 1, severity: Severity.Warning }) - } + }) + return diagnostics } diff --git a/src/rules/hasMacroParentheses.spec.ts b/src/rules/hasMacroParentheses.spec.ts index bb77ae7..49da46c 100644 --- a/src/rules/hasMacroParentheses.spec.ts +++ b/src/rules/hasMacroParentheses.spec.ts @@ -125,4 +125,18 @@ describe('hasMacroParentheses', () => { ]) }) }) + + it('should return an array with a single diagnostic when a macro definition contains a space', () => { + const content = `%macro test ()` + + expect(hasMacroParentheses.test(content)).toEqual([ + { + message: 'Macro definition contains space(s)', + lineNumber: 1, + startColumnNumber: 8, + endColumnNumber: 14, + severity: Severity.Warning + } + ]) + }) }) diff --git a/src/rules/hasMacroParentheses.ts b/src/rules/hasMacroParentheses.ts index fb894fa..6e9306b 100644 --- a/src/rules/hasMacroParentheses.ts +++ b/src/rules/hasMacroParentheses.ts @@ -4,23 +4,23 @@ import { LintRuleType } from '../types/LintRuleType' import { Severity } from '../types/Severity' import { trimComments } from '../utils/trimComments' import { getLineNumber } from '../utils/getLineNumber' -import { getColNumber } from '../utils/getColNumber' +import { getColumnNumber } from '../utils/getColumnNumber' const name = 'hasMacroParentheses' -const description = 'Macros are always defined with parentheses' +const description = 'Enforces the presence of parantheses in macro definitions.' const message = 'Macro definition missing parentheses' const test = (value: string) => { const diagnostics: Diagnostic[] = [] const statements: string[] = value ? value.split(';') : [] - let trimmedStatement = '', - commentStarted = false + let isCommentStarted = false statements.forEach((statement, index) => { - ;({ statement: trimmedStatement, commentStarted } = trimComments( + const { statement: trimmedStatement, commentStarted } = trimComments( statement, - commentStarted - )) + isCommentStarted + ) + isCommentStarted = commentStarted if (trimmedStatement.startsWith('%macro')) { const macroNameDefinition = trimmedStatement @@ -34,7 +34,7 @@ const test = (value: string) => { diagnostics.push({ message: 'Macro definition missing name', lineNumber: getLineNumber(statements, index + 1), - startColumnNumber: getColNumber(statement, '%macro'), + startColumnNumber: getColumnNumber(statement, '%macro'), endColumnNumber: statement.length, severity: Severity.Warning }) @@ -42,20 +42,20 @@ const test = (value: string) => { diagnostics.push({ message, lineNumber: getLineNumber(statements, index + 1), - startColumnNumber: getColNumber(statement, macroNameDefinition), + startColumnNumber: getColumnNumber(statement, macroNameDefinition), endColumnNumber: - getColNumber(statement, macroNameDefinition) + + getColumnNumber(statement, macroNameDefinition) + macroNameDefinition.length - 1, severity: Severity.Warning }) else if (macroName !== macroName.trim()) diagnostics.push({ - message: 'Macro definition cannot have space', + message: 'Macro definition contains space(s)', lineNumber: getLineNumber(statements, index + 1), - startColumnNumber: getColNumber(statement, macroNameDefinition), + startColumnNumber: getColumnNumber(statement, macroNameDefinition), endColumnNumber: - getColNumber(statement, macroNameDefinition) + + getColumnNumber(statement, macroNameDefinition) + macroNameDefinition.length - 1, severity: Severity.Warning @@ -66,7 +66,7 @@ const test = (value: string) => { } /** - * Lint rule that checks for the presence of macro name in %mend statement. + * Lint rule that enforces the presence of parantheses in macro definitions.. */ export const hasMacroParentheses: FileLintRule = { type: LintRuleType.File, diff --git a/src/rules/noNestedMacros.spec.ts b/src/rules/noNestedMacros.spec.ts index a965190..d77aa5b 100644 --- a/src/rules/noNestedMacros.spec.ts +++ b/src/rules/noNestedMacros.spec.ts @@ -11,7 +11,7 @@ describe('noNestedMacros', () => { expect(noNestedMacros.test(content)).toEqual([]) }) - it('should return an array with a single diagnostics when nested macro defined', () => { + it('should return an array with a single diagnostic when a macro contains a nested macro definition', () => { const content = ` %macro outer(); /* any amount of arbitrary code */ @@ -26,7 +26,7 @@ describe('noNestedMacros', () => { expect(noNestedMacros.test(content)).toEqual([ { - message: "Macro definition present inside another macro 'outer'", + message: "Macro definition for 'inner' present in macro 'outer'", lineNumber: 4, startColumnNumber: 7, endColumnNumber: 20, @@ -35,7 +35,7 @@ describe('noNestedMacros', () => { ]) }) - it('should return an array with a single diagnostics when nested macro defined 2 levels', () => { + it('should return an array with a single diagnostic when nested macros are defined at 2 levels', () => { const content = ` %macro outer(); /* any amount of arbitrary code */ @@ -54,14 +54,14 @@ describe('noNestedMacros', () => { expect(noNestedMacros.test(content)).toEqual([ { - message: "Macro definition present inside another macro 'outer'", + message: "Macro definition for 'inner' present in macro 'outer'", lineNumber: 4, startColumnNumber: 7, endColumnNumber: 20, severity: Severity.Warning }, { - message: "Macro definition present inside another macro 'inner'", + message: "Macro definition for 'inner2' present in macro 'inner'", lineNumber: 7, startColumnNumber: 17, endColumnNumber: 31, diff --git a/src/rules/noNestedMacros.ts b/src/rules/noNestedMacros.ts index 2fb5fc9..1acbd33 100644 --- a/src/rules/noNestedMacros.ts +++ b/src/rules/noNestedMacros.ts @@ -4,51 +4,53 @@ import { LintRuleType } from '../types/LintRuleType' import { Severity } from '../types/Severity' import { trimComments } from '../utils/trimComments' import { getLineNumber } from '../utils/getLineNumber' -import { getColNumber } from '../utils/getColNumber' +import { getColumnNumber } from '../utils/getColumnNumber' const name = 'noNestedMacros' -const description = 'Defining nested macro is not good practice' -const message = 'Macro definition present inside another macro' +const description = 'Enfoces the absence of nested macro definitions.' +const message = `Macro definition for '{macro}' present in macro '{parent}'` const test = (value: string) => { const diagnostics: Diagnostic[] = [] const statements: string[] = value ? value.split(';') : [] - const stack: string[] = [] - let trimmedStatement = '', - commentStarted = false + const declaredMacros: string[] = [] + let isCommentStarted = false statements.forEach((statement, index) => { - ;({ statement: trimmedStatement, commentStarted } = trimComments( + const { statement: trimmedStatement, commentStarted } = trimComments( statement, - commentStarted - )) + isCommentStarted + ) + isCommentStarted = commentStarted if (trimmedStatement.startsWith('%macro ')) { const macroName = trimmedStatement .slice(7, trimmedStatement.length) .trim() .split('(')[0] - if (stack.length) { - const parentMacro = stack.slice(-1).pop() + if (declaredMacros.length) { + const parentMacro = declaredMacros.slice(-1).pop() diagnostics.push({ - message: `${message} '${parentMacro}'`, + message: message + .replace('{macro}', macroName) + .replace('{parent}', parentMacro!), lineNumber: getLineNumber(statements, index + 1), - startColumnNumber: getColNumber(statement, '%macro'), + startColumnNumber: getColumnNumber(statement, '%macro'), endColumnNumber: - getColNumber(statement, '%macro') + trimmedStatement.length - 1, + getColumnNumber(statement, '%macro') + trimmedStatement.length - 1, severity: Severity.Warning }) } - stack.push(macroName) + declaredMacros.push(macroName) } else if (trimmedStatement.startsWith('%mend')) { - stack.pop() + declaredMacros.pop() } }) return diagnostics } /** - * Lint rule that checks for the presence of macro name in %mend statement. + * Lint rule that checks for the absence of nested macro definitions. */ export const noNestedMacros: FileLintRule = { type: LintRuleType.File,