Skip to content
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.

Sign up for GitHub

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

no-default-id-values rule #137

Merged
merged 4 commits into from
Dec 11, 2024
Merged

no-default-id-values rule #137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0804d20
Initial implementation of no-default-values rule
fabioh8010 Nov 22, 2024
98b7777
Refactor code and add tests
fabioh8010 Nov 28, 2024
b9aec0a
Rename ESLint rule
fabioh8010 Nov 28, 2024
f784aff
Move functions outside create()
fabioh8010 Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions eslint-plugin-expensify/no-default-id-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
function createPatternRegex(pattern) {
return new RegExp(pattern.replace(/([.?*+^$[\]\\(){}|-])/g, '\\$1'), 'g');
}

function searchForPatternsAndReport(context, sourceCode, soureCodeStr, pattern, messageId) {
const regex = createPatternRegex(pattern);
let match = regex.exec(soureCodeStr);
while (match !== null) {
const index = match.index;

const defaultStr = match[0];
const defaultStrPosition = sourceCode.getLocFromIndex(index);

context.report({
messageId,
loc: {
start: {line: defaultStrPosition.line, column: defaultStrPosition.column + defaultStr.indexOf(' ')},
end: {line: defaultStrPosition.line, column: defaultStrPosition.column + defaultStr.length},
},
});

match = regex.exec(soureCodeStr);
}
}

module.exports = {
name: 'no-default-id-values',
meta: {
type: 'problem',
docs: {
description: 'Restricts use of default number/string IDs in the project.',
recommended: 'error',
},
schema: [],
messages: {
// eslint-disable-next-line max-len
disallowedNumberDefault: 'Default the number ID to `CONST.DEFAULT_NUMBER_ID` instead. See: https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#default-value-for-inexistent-IDs',
// eslint-disable-next-line max-len
disallowedStringDefault: 'Do not default string IDs to any value. See: https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#default-value-for-inexistent-IDs',
},
},
create(context) {
const sourceCode = context.getSourceCode();
const soureCodeStr = sourceCode.text; // This gets all the text in the file

const disallowedNumberDefaults = [
'ID ?? -1',
'id ?? -1',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than having all case-sensitive options, can we just make the check be cause-insensitive?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabioh8010 bump on this question

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep them all separated for all, just in case we discover that some string match is being problematic, but it happens to be case sensitive. Is that ok for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, yeah. That's fine. Thanks for explaining and documenting the analysis you did!

'ID ?? 0',
'id ?? 0',
'ID || -1',
'id || -1',
'ID || 0',
'id || 0',
'ID : -1',
'id : -1',
'ID : 0',
'id : 0',
];

const disallowedStringDefaults = [
" ?? '-1'",
"ID ?? ''",
"id ?? ''",
"ID ?? '0'",
"id ?? '0'",
" || '-1'",
"ID || ''",
"id || ''",
"ID || '0'",
"id || '0'",
" : '-1'",
" : '0'",
];

disallowedNumberDefaults.forEach((pattern) => {
searchForPatternsAndReport(context, sourceCode, soureCodeStr, pattern, 'disallowedNumberDefault');
});

disallowedStringDefaults.forEach((pattern) => {
searchForPatternsAndReport(context, sourceCode, soureCodeStr, pattern, 'disallowedStringDefault');
});

return {};
},
};
260 changes: 260 additions & 0 deletions eslint-plugin-expensify/tests/no-default-id-values.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,260 @@
const RuleTester = require('eslint').RuleTester;
const rule = require('../no-default-id-values');

const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 2020,
sourceType: 'module',
},
});

ruleTester.run('no-default-id-values', rule, {
valid: [
// Number IDs
{
code: 'const accountID = report?.ownerAccountID ?? CONST.DEFAULT_NUMBER_ID;',
},
{
code: 'const accountID = account?.id ?? CONST.DEFAULT_NUMBER_ID;',
},
{
code: 'currentState = currentState?.routes[currentState.index ?? -1].state;',
},
{
code: 'currentState = currentState?.routes[currentState.index ?? 0].state;',
},
{
code: 'const accountID = report?.ownerAccountID || CONST.DEFAULT_NUMBER_ID;',
},
{
code: 'const accountID = account?.id || CONST.DEFAULT_NUMBER_ID;',
},
{
code: 'currentState = currentState?.routes[currentState.index || -1].state;',
},
{
code: 'currentState = currentState?.routes[currentState.index || 0].state;',
},
{
code: 'const managerID = report ? report.managerID : CONST.DEFAULT_NUMBER_ID;',
},
{
code: 'const accountID = account ? account.id : CONST.DEFAULT_NUMBER_ID;',
},
{
code: 'options.sort((method) => (method.value === exportMethod ? -1 : 0));',
},

// String IDs
{
code: 'const reportID = report?.reportID;',
},
{
code: 'const iconName = icon.name ?? \'\'',
},
{
code: 'const index = tempIndex ?? \'0\'',
},
{
code: 'const iconName = icon.name || \'\'',
},
{
code: 'const index = tempIndex || \'0\'',
},
],
invalid: [
// Number IDs
{
code: 'const accountID = report?.ownerAccountID ?? -1;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const reportID = report?.id ?? -1;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const accountID = report?.ownerAccountID ?? 0;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const reportID = report?.id ?? 0;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const accountID = report?.ownerAccountID || -1;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const reportID = report?.id || -1;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const accountID = report?.ownerAccountID || 0;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const reportID = report?.id || 0;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const managerID = report ? report.managerID : -1;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const accountID = account ? account.id : -1;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const managerID = report ? report.managerID : 0;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},
{
code: 'const accountID = account ? account.id : 0;',
errors: [{
messageId: 'disallowedNumberDefault',
}],
},

// String IDs
{
code: 'const reportID = report?.reportID ?? \'-1\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const currentReportID = Navigation.getTopmostReportId() ?? \'-1\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const currentReportID = navigationRef?.isReady?.() ? Navigation.getTopmostReportId() ?? \'-1\' : \'-1\';',
errors: [
{
messageId: 'disallowedStringDefault',
},
{
messageId: 'disallowedStringDefault',
},
],
},
{
code: 'const reportID = report?.reportID ?? \'-1\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const policyID = policy?.id ?? \'-1\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const reportID = report?.reportID ?? \'\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const policyID = policy?.id ?? \'\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const reportID = report?.reportID ?? \'0\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const policyID = policy?.id ?? \'0\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const reportID = report?.reportID || \'-1\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const currentReportID = Navigation.getTopmostReportId() || \'-1\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const currentReportID = navigationRef?.isReady?.() ? Navigation.getTopmostReportId() || \'-1\' : \'-1\';',
errors: [
{
messageId: 'disallowedStringDefault',
},
{
messageId: 'disallowedStringDefault',
},
],
},
{
code: 'const reportID = report?.reportID || \'\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const policyID = policy?.id || \'\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const reportID = report?.reportID || \'0\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const policyID = policy?.id || \'0\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const reportID = report ? report.reportID : \'-1\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
{
code: 'const reportID = report ? report.reportID : \'0\';',
errors: [{
messageId: 'disallowedStringDefault',
}],
},
],
});
Loading