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.

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 check to use periods at the end of error messages #88

Merged
merged 4 commits into from
Apr 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
1 change: 1 addition & 0 deletions eslint-plugin-expensify/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ module.exports = {
ONYX_ONE_PARAM: 'The withOnyx HOC must be passed at least one argument.',
MUST_USE_VARIABLE_FOR_ASSIGNMENT: '{{key}} must be assigned as a variable instead of direct assignment.',
NO_DEFAULT_PROPS: 'defaultProps should not be used in function components. Use default Arguments instead.',
USE_PERIODS_ERROR_MESSAGES: 'Use periods at the end of error messages.'
},
};
38 changes: 38 additions & 0 deletions eslint-plugin-expensify/tests/use-periods-error-messages.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
const RuleTester = require('eslint').RuleTester;
const rule = require('../use-periods-for-error-messages');
const message = require('../CONST').MESSAGE.USE_PERIODS_ERROR_MESSAGES;

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

const goodExample = `
error: {
testMessage: 'This is a test message.'
}
`;

const badExample = `
error: {
testMessage: 'This is a test message'
}
`;

ruleTester.run('use-periods-for-error-messages', rule, {
valid: [
{
code: goodExample,
},
],
invalid: [
{
code: badExample,
errors: [{
message,
}],
},
],
});
31 changes: 31 additions & 0 deletions eslint-plugin-expensify/use-periods-for-error-messages.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
require('lodash/get');
const message = require('./CONST').MESSAGE.USE_PERIODS_ERROR_MESSAGES;

module.exports = {
create(context) {
return {
Property(node) {
if (!node.key || node.key.name !== 'error' || !node.value || node.value.type !== 'ObjectExpression') {
return;
}
node.value.properties.forEach((property) => {
if (!property.value || property.value.type !== 'Literal' || typeof property.value.value !== 'string') {
return;
}
const errorMessage = property.value.value;
if (!errorMessage.endsWith('.')) {
context.report({
node: property,
message,
fix: function (fixer) {
const lastChar = errorMessage[errorMessage.length - 1];
const fixedMessage = lastChar === '.' ? errorMessage : `${errorMessage}.`;
Copy link

Choose a reason for hiding this comment

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

i don't see this condition working while having it inside if (!errorMessage.endsWith('.')) {, am i missing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the check was not needed. Updated it.

return fixer.replaceText(property.value, `'${fixedMessage}'`);
}
});
}
});
},
};
},
};
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "eslint-config-expensify",
"version": "2.0.44",
"version": "2.0.45",
Copy link

Choose a reason for hiding this comment

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

Do we need to change this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we need to increase the version and then use it in Expensify/App.

Copy link

Choose a reason for hiding this comment

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

I don't think that's how it works, check other PRs. that step seems to be automated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/Expensify/eslint-config-expensify/pull/54/files#diff-053150b640a7ce75eff69d1a22cae7f0f94ad64ce9a855db544dda0929316519

It's there in above PR. It needs to be done in package-lock.json also, added there as well.

Copy link

Choose a reason for hiding this comment

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

Can you check recent PRs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, OSBotify bot will update it. Updated.

"description": "Expensify's ESLint configuration following our style guide",
"main": "index.js",
"repository": {
Expand Down