-
Notifications
You must be signed in to change notification settings - Fork 4
NEW @W-19772057@ Enhanced rule selector support #363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
d188bb3
c46b989
5fd3cbb
24a8379
42d58cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -163,6 +163,12 @@ const MESSAGE_CATALOG : MessageCatalog = { | |||||
| RuleDoesNotExistInSelection: | ||||||
| `No rule with name '%s' and engine '%s' exists among the selected rules.`, | ||||||
|
|
||||||
| SelectorCannotBeEmpty: | ||||||
| `Rule selectors cannot be empty strings.`, | ||||||
|
|
||||||
| SelectorLooksIncorrect: | ||||||
| `Rule selector '%s' looks incorrect. Make sure that parentheses are balanced, and that all subselectors are joined by a colon (:) or comma (,).`, | ||||||
|
||||||
| `Rule selector '%s' looks incorrect. Make sure that parentheses are balanced, and that all subselectors are joined by a colon (:) or comma (,).`, | |
| `Rule selector '%s' looks incorrect. Make sure that parentheses are balanced and that all subselectors are joined by a colon (:) or comma (,).`, |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `Could to get results for engine '%s' since they are missing from the overall run results. Most likely the engine did not run.`, | |
| `Couldn't get results for engine '%s' since they're missing from the overall run results. Most likely the engine didn't run.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice spot @jshackell-sfdc!
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| import { getMessage } from "./messages"; | ||
|
|
||
| export interface Selector { | ||
| matchesSelectables(selectables: string[]): boolean; | ||
| } | ||
|
|
||
| export function toSelector(selectorString: string): Selector { | ||
| if (selectorString === '') { | ||
| throw new Error(getMessage("SelectorCannotBeEmpty")); | ||
| } else if (selectorString.endsWith(')')) { | ||
| const correspondingOpenParen: number = identifyCorrespondingOpenParen(selectorString); | ||
| if (correspondingOpenParen === 0) { | ||
| return toSelector(selectorString.slice(1, -1)) | ||
| } else { | ||
| const left: string = selectorString.slice(0, correspondingOpenParen - 1); | ||
| const right: string = selectorString.slice(correspondingOpenParen); | ||
| const op: string = selectorString[correspondingOpenParen - 1]; | ||
| return toComplexSelector(left, right, op); | ||
| } | ||
| } else { | ||
| const lastComma: number = selectorString.lastIndexOf(','); | ||
| const lastColon: number = selectorString.lastIndexOf(':'); | ||
|
|
||
| if (lastComma === -1 && lastColon === -1) { | ||
| if (selectorString.includes(')') || selectorString.includes('(')) { | ||
| throw new Error(getMessage('SelectorLooksIncorrect', selectorString)); | ||
| } | ||
| return new SimpleSelector(selectorString); | ||
| } else if (lastComma > lastColon) { | ||
| const left: string = selectorString.slice(0, lastComma); | ||
| const right: string = selectorString.slice(lastComma + 1); | ||
| return toComplexSelector(left, right, ','); | ||
| } else { | ||
| const left: string = selectorString.slice(0, lastColon); | ||
| const right: string = selectorString.slice(lastColon + 1); | ||
| return toComplexSelector(left, right, ':'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function identifyCorrespondingOpenParen(selectorString: string): number { | ||
| const reversedLetters: string[] = selectorString.split('').reverse(); | ||
| let parenBalance: number = 0; | ||
| let idx = 0; | ||
| for (const letter of reversedLetters) { | ||
| if (letter === ')') { | ||
| parenBalance += 1; | ||
| } else if (letter === '(') { | ||
| parenBalance -= 1; | ||
| } | ||
| if (parenBalance === 0) { | ||
| break; | ||
| } | ||
| idx += 1; | ||
| } | ||
|
|
||
| if (parenBalance > 0) { | ||
| throw new Error(getMessage("SelectorLooksIncorrect", selectorString)); | ||
| } | ||
|
|
||
| return selectorString.length - idx - 1; | ||
| } | ||
|
|
||
| function toComplexSelector(left: string, right: string, op: string): Selector { | ||
| if (op === ',') { | ||
| return new OrSelector(toSelector(left), toSelector(right)); | ||
| } else if (op === ':') { | ||
| return new AndSelector(toSelector(left), toSelector(right)); | ||
| } else { | ||
| throw new Error(getMessage("SelectorLooksIncorrect", `${left}${op}${right}`)); | ||
| } | ||
| } | ||
|
|
||
| class SimpleSelector implements Selector { | ||
| private readonly selector: string; | ||
|
|
||
| constructor(selector: string) { | ||
| this.selector = selector; | ||
| } | ||
|
|
||
| public matchesSelectables(selectables: string[]): boolean { | ||
| return selectables.some(s => s === this.selector.toLowerCase()); | ||
| } | ||
| } | ||
|
|
||
| class AndSelector implements Selector { | ||
| private readonly left: Selector; | ||
| private readonly right: Selector; | ||
|
|
||
| constructor(left: Selector, right: Selector) { | ||
| this.left = left; | ||
| this.right = right; | ||
| } | ||
|
|
||
| public matchesSelectables(selectables: string[]): boolean { | ||
| return this.left.matchesSelectables(selectables) && this.right.matchesSelectables(selectables); | ||
| } | ||
| } | ||
|
|
||
| class OrSelector implements Selector { | ||
| private readonly left: Selector; | ||
| private readonly right: Selector; | ||
|
|
||
| constructor(left: Selector, right: Selector) { | ||
| this.left = left; | ||
| this.right = right; | ||
| } | ||
|
|
||
| public matchesSelectables(selectables: string[]): boolean { | ||
| return this.left.matchesSelectables(selectables) || this.right.matchesSelectables(selectables); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -149,12 +149,21 @@ describe('Tests for selecting rules', () => { | |||||
| expect(ruleNamesFor(selection7,'stubEngine2')).toEqual([]) | ||||||
| }); | ||||||
|
|
||||||
| it('When multiple selectors are provided, then they act as a union', async () => { | ||||||
| const selection: RuleSelection = await codeAnalyzer.selectRules([ | ||||||
| 'Security', // a tag | ||||||
| 'stubEngine2', // an engine name | ||||||
| 'stub1RuleD' // a rule name | ||||||
| ]); | ||||||
| it.each([ | ||||||
| { | ||||||
| case: 'multiple selectors are provided', | ||||||
| selectors: [ | ||||||
| 'Security', // a tag | ||||||
| 'stubEngine2', // an engine name | ||||||
| 'stub1RuleD' // a rule name | ||||||
| ] | ||||||
| }, | ||||||
| { | ||||||
| case: 'using a comma to join two rule selectors', | ||||||
| selectors: ['Security,stubEngine2,stub1RuleD'] | ||||||
| } | ||||||
| ])('When $case, then it acts like a union', async ({selectors}) => { | ||||||
| const selection: RuleSelection = await codeAnalyzer.selectRules(selectors); | ||||||
|
|
||||||
| expect(selection.getEngineNames()).toEqual(['stubEngine1', 'stubEngine2']); | ||||||
| expect(ruleNamesFor(selection, 'stubEngine1')).toEqual(['stub1RuleB', 'stub1RuleD']); | ||||||
|
|
@@ -164,14 +173,76 @@ describe('Tests for selecting rules', () => { | |||||
| expect(await codeAnalyzer.selectRules(['all', 'Performance', 'DoesNotExist'])).toEqual(await codeAnalyzer.selectRules(['all'])); | ||||||
| }); | ||||||
|
|
||||||
| it('When colons are used and multiple selectors are provided then we get correct union and intersection behavior', async () => { | ||||||
| const selection: RuleSelection = await codeAnalyzer.selectRules(['Recommended:Performance', 'stubEngine2:2', 'stubEngine2:DoesNotExist']); | ||||||
| it.each([ | ||||||
| { | ||||||
| selector: 'Recommended:Performance,2', // Equivalent to "(Recommended:Performance),2", or "(Recommended,2):(Performance,2)" | ||||||
| engines: ['stubEngine1', 'stubEngine2'], | ||||||
| stubEngine1Rules: ['stub1RuleB', 'stub1RuleC'], | ||||||
| stubEngine2Rules: ['stub2RuleC'] | ||||||
| }, | ||||||
| { | ||||||
| selector: 'Recommended,3:Performance', // Equivalent to "(Recommended,4):Performance", or "(Recommended:Performance),(4:Performance)" | ||||||
|
||||||
| selector: 'Recommended,3:Performance', // Equivalent to "(Recommended,4):Performance", or "(Recommended:Performance),(4:Performance)" | |
| selector: 'Recommended,3:Performance', // Equivalent to "(Recommended,3):Performance", or "(Recommended:Performance),(3:Performance)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, good catch. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a logical limit to how many unions and intersections there can be, or does the limit not exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithm is just standard recursion, so there's no hardcoded limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.