Skip to content

Commit

Permalink
fix(no-input-prefix): exact strings not being reported (#597)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rafaelss95 @mgechev
rafaelss95 authored and mgechev committed May 4, 2018
1 parent 09c1309 commit 1ed8d8c
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 54 deletions.
16 changes: 7 additions & 9 deletions src/noInputPrefixRule.ts
View file
@@ -1,6 +1,5 @@
import { sprintf } from 'sprintf-js';
import { IOptions, IRuleMetadata, RuleFailure, Rules } from 'tslint/lib';
import { arrayify } from 'tslint/lib/utils';
import { Decorator, Node, PropertyAccessExpression, PropertyDeclaration, SourceFile } from 'typescript';

import { NgWalker } from './angular/ngWalker';
Expand All @@ -14,15 +13,15 @@ export class Rule extends Rules.AbstractRule {
type: 'array'
},
optionsDescription: 'Options accept a string array of disallowed input prefixes.',
rationale: `HTML attributes are not prefixed. It's considered best not to prefix Inpu
rationale: `HTML attributes are not prefixed. It's considered best not to prefix Inputs.
* Example: 'enabled' is prefered over 'isEnabled'.
`,
ruleName: 'no-input-prefix',
type: 'maintainability',
typescriptOnly: true
};

static readonly FAILURE_STRING = 'In the class "%s", the input property "%s" should not be prefixed by %s';
static readonly FAILURE_STRING = '@Inputs should not be prefixed by %s';

apply(sourceFile: SourceFile): RuleFailure[] {
return this.applyWithWalker(new NoInputPrefixWalker(sourceFile, this.getOptions()));
Expand All @@ -42,16 +41,16 @@ const getReadablePrefixes = (prefixes: string[]): string => {
.join(', ')} or "${[...prefixes].pop()}"`;
};

export const getFailureMessage = (className: string, propertyName: string, prefixes: string[]): string => {
return sprintf(Rule.FAILURE_STRING, className, propertyName, getReadablePrefixes(prefixes));
export const getFailureMessage = (prefixes: string[]): string => {
return sprintf(Rule.FAILURE_STRING, getReadablePrefixes(prefixes));
};

class NoInputPrefixWalker extends NgWalker {
private readonly blacklistedPrefixes: string[];

constructor(source: SourceFile, options: IOptions) {
super(source, options);
this.blacklistedPrefixes = arrayify<string>(options.ruleArguments).slice(1);
this.blacklistedPrefixes = options.ruleArguments.slice(1);
}

protected visitNgInput(property: PropertyDeclaration, input: Decorator, args: string[]) {
Expand All @@ -61,14 +60,13 @@ class NoInputPrefixWalker extends NgWalker {

private validatePrefix(property: PropertyDeclaration, input: Decorator, args: string[]) {
const memberName = property.name.getText();
const isBlackListedPrefix = this.blacklistedPrefixes.some(x => new RegExp(`^${x}[^a-z]`).test(memberName));
const isBlackListedPrefix = this.blacklistedPrefixes.some(x => x === memberName || new RegExp(`^${x}[^a-z]`).test(memberName));

if (!isBlackListedPrefix) {
return;
}

const className = (property.parent as PropertyAccessExpression).name.getText();
const failure = getFailureMessage(className, memberName, this.blacklistedPrefixes);
const failure = getFailureMessage(this.blacklistedPrefixes);

this.addFailureAtNode(property, failure);
}
Expand Down
96 changes: 51 additions & 45 deletions test/noInputPrefixRule.spec.ts
View file
Expand Up @@ -5,89 +5,93 @@ const {
FAILURE_STRING,
metadata: { ruleName }
} = Rule;
const className = 'Test';

const getFailureAnnotations = (num: number): string => {
return '~'.repeat(num);
};

const getComposedOptions = (prefixes: string[]): (boolean | string)[] => {
return [true, ...prefixes];
const getComposedOptions = (blacklistedPrefixes: string[]): (boolean | string)[] => {
return [true, ...blacklistedPrefixes];
};

describe(ruleName, () => {
describe('failure', () => {
it('should fail when an input property is prefixed by a blacklisted prefix and blacklist is composed by one prefix', () => {
const prefixes = ['is'];
const propertyName = `${prefixes[0]}Disabled`;
const inputExpression = `@Input() ${propertyName}: boolean;`;
const blacklistedPrefixes = ['is'];
const source = `
@Directive()
class Test {
@Input() isDisabled: boolean;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
}
`;
assertAnnotated({
message: getFailureMessage(blacklistedPrefixes),
options: getComposedOptions(blacklistedPrefixes),
ruleName,
source
});
});

it('should fail when an input property is strictly equal to a blacklisted prefix', () => {
const blacklistedPrefixes = ['should'];
const source = `
@Directive()
class ${className} {
${inputExpression}
${getFailureAnnotations(inputExpression.length)}
class Test {
@Input() should: boolean;
~~~~~~~~~~~~~~~~~~~~~~~~~
}
`;
assertAnnotated({
message: getFailureMessage(className, propertyName, prefixes),
options: getComposedOptions(prefixes),
message: getFailureMessage(blacklistedPrefixes),
options: getComposedOptions(blacklistedPrefixes),
ruleName,
source
});
});

it('should fail when an input property is prefixed by a blacklisted prefix and blacklist is composed by two prefixes', () => {
const prefixes = ['can', 'is'];
const propertyName = `${prefixes[0]}Enable`;
const inputExpression = `@Input() ${propertyName}: boolean;`;
it('should fail when an input property is prefixed by a blacklisted prefix and blacklist is composed by two blacklistedPrefixes', () => {
const blacklistedPrefixes = ['can', 'is'];
const source = `
@Component()
class ${className} {
${inputExpression}
${getFailureAnnotations(inputExpression.length)}
class Test {
@Input() canEnable: boolean;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
}
`;
assertAnnotated({
message: getFailureMessage(className, propertyName, prefixes),
options: getComposedOptions(prefixes),
message: getFailureMessage(blacklistedPrefixes),
options: getComposedOptions(blacklistedPrefixes),
ruleName,
source
});
});

it('should fail when an input property is prefixed by a blacklisted prefix and blacklist is composed by two concurrent prefixes', () => {
const prefixes = ['is', 'isc'];
const propertyName = `${prefixes[1]}Hange`;
const inputExpression = `@Input() ${propertyName}: boolean;`;
it('should fail when an input property is prefixed by a blacklisted prefix and blacklist is composed by two concurrent blacklistedPrefixes', () => {
const blacklistedPrefixes = ['is', 'isc'];
const source = `
@Component()
class ${className} {
${inputExpression}
${getFailureAnnotations(inputExpression.length)}
class Test {
@Input() iscHange: boolean;
~~~~~~~~~~~~~~~~~~~~~~~~~~~
}
`;
assertAnnotated({
message: getFailureMessage(className, propertyName, prefixes),
options: getComposedOptions(prefixes),
message: getFailureMessage(blacklistedPrefixes),
options: getComposedOptions(blacklistedPrefixes),
ruleName,
source
});
});

it('should fail when an input property is snakecased and contains a blacklisted prefix', () => {
const prefixes = ['do'];
const propertyName = `${prefixes[0]}_it`;
const inputExpression = `@Input() ${propertyName}: number;`;
const blacklistedPrefixes = ['do'];
const source = `
@Directive()
class ${className} {
${inputExpression}
${getFailureAnnotations(inputExpression.length)}
class Test {
@Input() do_it: number;
~~~~~~~~~~~~~~~~~~~~~~~
}
`;
assertAnnotated({
message: getFailureMessage(className, propertyName, prefixes),
options: getComposedOptions(prefixes),
message: getFailureMessage(blacklistedPrefixes),
options: getComposedOptions(blacklistedPrefixes),
ruleName,
source
});
Expand All @@ -96,26 +100,28 @@ describe(ruleName, () => {

describe('success', () => {
it('should succeed when an input property is not prefixed', () => {
const blacklistedPrefixes = ['must'];
const source = `
@Directive()
class ${className} {
class Test {
@Input() mustmust = true;
}
`;
assertSuccess(ruleName, source, getComposedOptions(['must']));
assertSuccess(ruleName, source, getComposedOptions(blacklistedPrefixes));
});

it('should succeed when multiple input properties are prefixed by something not present in the blacklist', () => {
const blacklistedPrefixes = ['can', 'dis', 'disable', 'should'];
const source = `
@Component()
class ${className} {
class Test {
@Input() cana: string;
@Input() disabledThing: boolean;
@Input() isFoo = 'yes';
@Input() shoulddoit: boolean;
}
`;
assertSuccess(ruleName, source, getComposedOptions(['can', 'should', 'dis', 'disable']));
assertSuccess(ruleName, source, getComposedOptions(blacklistedPrefixes));
});
});
});

0 comments on commit 1ed8d8c

Please sign in to comment.