-
Notifications
You must be signed in to change notification settings - Fork 181
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
Add a sanity check for rules that should be mapped #4350
Conversation
c338692
to
fee37a8
Compare
fee37a8
to
b157605
Compare
Quality Gate passedKudos, no new issues are introduced in this PR! 0 New issues |
const { rule } = require(path.join(ruleFolder, sonarKey)); | ||
if (!mappedRules.some(mapped => mapped === rule)) { | ||
missing.push(sonarKey); | ||
} |
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.
why wouldn't this be enough?
const { rule } = require(path.join(ruleFolder, sonarKey)); | |
if (!mappedRules.some(mapped => mapped === rule)) { | |
missing.push(sonarKey); | |
} | |
if (!mappedRules[sonarKey]) { | |
missing.push(sonarKey); | |
} |
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 check we want to implement is that we don't forget to add the rule redefinition/decoration/whatever, so it's the case where we:
- write it, and its units tests in
packages/src/ rules/Sxxxx/
- forget to load it in
packages/src/rules/index.ts
No?
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.
As I was saying in my comment, we are not able to do what you are suggesting because the keys in the mapping aren't Sonar keys:
SonarJS/packages/jsts/src/rules/index.ts
Lines 300 to 302 in 0b48c18
rules['accessor-pairs'] = S2376; | |
rules['anchor-has-content'] = S6827; | |
rules['anchor-precedence'] = S5850; |
We don't have rules['S2376'] = S2376;
but rules['accessor-pairs'] = S2376;
instead. This is why we need to check the other way around.
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.
thanks for the explanation
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.
You're welcome :)
|
||
describe('index', () => { | ||
it('should map keys to rules definitions', () => { | ||
// FIXME: This test runs with a time complexity of O(n^2) where n is the number of rules. |
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.
Isn't it O(n)?
- gather
sonarKeys
: n - gather
mappedRules
: n - find missing: n
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 current implementation is O(n^2) since we iterate with the for-loop O(n) over all the rules available in the folder rules/SXXX
, and for each of them, we look for a matching mapped definition with mappedRules.some(mapped => mapped === rule))
O(n).
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.
indeed
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.
I think you can simplify it as per my comments. I'm open to discuss it if you think we need a more thorough check, but I don't see what it brings, except checking that require works.
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.
LGTM!
No description provided.