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

Automate Linting with Husky and lint-staged to Prevent CI Failures #896

Merged
merged 13 commits into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
10 changes: 10 additions & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# common
**/dist/*

# sdk
packages/sdk/src/api/yorkie/v1/yorkie_grpc_web_pb.d.ts
packages/sdk/src/api/yorkie/v1/yorkie_pb.d.ts
packages/sdk/src/api/yorkie/v1/resources_grpc_web_pb.d.ts
packages/sdk/src/api/yorkie/v1/resources_pb.d.ts
packages/sdk/test/vitest.d.ts
packages/sdk/lib
61 changes: 61 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
module.exports = {
root: true,
plugins: ['prettier', 'jsdoc', '@typescript-eslint'],
extends: [
'eslint:recommended',
'plugin:@typescript-eslint/eslint-recommended',
'plugin:@typescript-eslint/recommended',
],
rules: {
'prettier/prettier': 'error',
'object-shorthand': ['error', 'always'],
'no-unreachable': 'error',
},
overrides: [
{
files: ['**/*.ts', '**/*.tsx'],
parser: '@typescript-eslint/parser',
plugins: ['@typescript-eslint'],
extends: ['plugin:@typescript-eslint/recommended'],
rules: {
'@typescript-eslint/no-non-null-assertion': 'off',
'@typescript-eslint/no-explicit-any': 'off',
'@typescript-eslint/ban-ts-comment': 'off',
'jsdoc/require-jsdoc': [
'error',
{
contexts: ['MethodDefinition:not([accessibility="private"])'],
require: {
ClassDeclaration: true,
},
checkConstructors: false,
enableFixer: false,
},
],
'@typescript-eslint/naming-convention': [
'error',
{
selector: 'variable',
format: ['camelCase', 'PascalCase'],
leadingUnderscore: 'allowDouble',
trailingUnderscore: 'allowDouble',
},
],
'@typescript-eslint/ban-types': [
'error',
{
types: { null: 'Use undefined instead of null' },
},
],
'@typescript-eslint/array-type': ['error', { default: 'generic' }],
'@typescript-eslint/no-this-alias': [
'error',
{
allowDestructuring: true,
allowedNames: ['node'],
},
],
},
},
],
};
Comment on lines +1 to +61
Copy link

Choose a reason for hiding this comment

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

Review of ESLint Configuration: Generally Solid with Suggestions for Improvement

The ESLint configuration is well-structured and covers a broad range of rules that are essential for maintaining code quality and consistency, especially in a TypeScript project. Here are some observations and suggestions:

  • TypeScript Rules: The configuration disables some TypeScript rules (@typescript-eslint/no-non-null-assertion, @typescript-eslint/no-explicit-any, and @typescript-eslint/ban-ts-comment). While this may offer flexibility, it could compromise type safety. Consider enabling these rules if the project aims for strict type safety.
  • JSDoc Rules: The inclusion of JSDoc rules (jsdoc/require-jsdoc) is commendable as it enforces documentation, which is crucial for maintainability and clarity.
  • Prettier Integration: Enforcing prettier rules as errors is a good practice to ensure consistent code formatting.

Overall, the setup is robust, but revisiting the disabled TypeScript rules could further enhance the project's code quality.

2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ jobs:
if: steps.cache.outputs.cache-hit != 'true'
run: pnpm install

- run: pnpm sdk lint
- run: pnpm lint
- run: pnpm sdk build
- run: pnpm build:examples
- run: docker compose -f docker/docker-compose-ci.yml up --build -d
Expand Down
1 change: 1 addition & 0 deletions .husky/pre-commit
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
npx lint-staged
1 change: 1 addition & 0 deletions examples/nextjs-scheduler/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@ module.exports = {
endOfLine: 'auto',
},
],
'@next/next/no-html-link-for-pages': 'off',
},
};
1 change: 0 additions & 1 deletion examples/nextjs-scheduler/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
"@types/react-dom": "18.2.0",
"eslint-config-next": "^14.2.5",
"eslint-config-prettier": "^9.1.0",
"eslint-plugin-prettier": "^5.0.0",
"prettier": "^3.3.3",
"typescript": "5.3.3"
}
Expand Down
39 changes: 39 additions & 0 deletions lint-staged.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
const { ESLint } = require('eslint');
const { execSync } = require('child_process');
const path = require('path');

const removeIgnoredFiles = async (files) => {
const eslintIgnorePath = path.resolve('.eslintignore'); // Pointing to the root .eslintignore
const eslint = new ESLint({ ignorePath: eslintIgnorePath });

const isIgnored = await Promise.all(
files.map(async (file) => {
const ignored = await eslint.isPathIgnored(file);
return ignored;
}),
);

const filteredFiles = files.filter((_, i) => !isIgnored[i]);
return filteredFiles;
};

module.exports = {
'**/*.ts': async (files) => {
const filesToLint = await removeIgnoredFiles(files);

if (filesToLint.length > 0) {
const fileArgs = filesToLint.join(' ');
const command = `pnpm exec eslint ${fileArgs} --fix --max-warnings=0 --ext .ts`;
try {
execSync(command, { stdio: 'inherit' });
process.exit(0);
} catch (error) {
console.error('Linting failed. Commit will be aborted.');
process.exit(1);
}
} else {
console.log('No eligible files to lint. Skipping lint-staged command.');
process.exit(0);
}
},
};
Comment on lines +20 to +39
Copy link

Choose a reason for hiding this comment

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

Robust linting configuration for TypeScript files.

The module export configuration for linting TypeScript files is well-implemented. The use of pnpm exec eslint with the --fix and --max-warnings=0 flags ensures that linting issues are automatically fixed and that no warnings are allowed.

Suggestion: Consider handling the process exit more gracefully by allowing other middleware in lint-staged to continue running if needed, rather than using process.exit() which stops all processes immediately.

11 changes: 9 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
"vanilla-codemirror6": "pnpm --filter=vanilla-codemirror6",
"vanilla-quill": "pnpm --filter=vanilla-quill",
"vuejs-kanban": "pnpm --filter=vuejs-kanban",
"build:examples": "pnpm --filter './examples/*' run build"
"build:examples": "pnpm --filter './examples/*' run build",
"lint": "eslint . --fix --max-warnings=0 --ext .ts",
"prepare": "husky"
},
"keywords": [],
"author": {
Expand All @@ -25,6 +27,11 @@
},
"license": "Apache-2.0",
"devDependencies": {
"only-allow": "^1.2.1"
"eslint": "^8.19.0",
"husky": "^9.1.5",
"lint-staged": "^15.2.9",
"only-allow": "^1.2.1",
"eslint-plugin-jsdoc": "^39.3.3",
"eslint-plugin-prettier": "^5.0.0"
}
}
7 changes: 0 additions & 7 deletions packages/sdk/.eslintignore

This file was deleted.

50 changes: 2 additions & 48 deletions packages/sdk/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,55 +1,9 @@
// eslint-disable-next-line no-undef
module.exports = {
root: true,
parser: '@typescript-eslint/parser',
plugins: ['@typescript-eslint', 'prettier', 'eslint-plugin-tsdoc', 'jsdoc'],
extends: [
'eslint:recommended',
'plugin:@typescript-eslint/eslint-recommended',
'plugin:@typescript-eslint/recommended',
],
extends: ['../../.eslintrc.js'],
plugins: ['eslint-plugin-tsdoc'],
rules: {
'prettier/prettier': 'error',
'@typescript-eslint/naming-convention': [
'error',
{
selector: 'variable',
format: ['camelCase', 'PascalCase'],
leadingUnderscore: 'allowDouble',
trailingUnderscore: 'allowDouble',
},
],
'@typescript-eslint/no-non-null-assertion': 'off',
'@typescript-eslint/no-explicit-any': 'off',
'@typescript-eslint/ban-ts-comment': 'off',
'@typescript-eslint/ban-types': [
'error',
{
types: { null: 'Use undefined instead of null' },
},
],
'@typescript-eslint/array-type': ['error', { default: 'generic' }],
'tsdoc/syntax': 'error',
'object-shorthand': ['error', 'always'],
'no-unreachable': 'error',
'jsdoc/require-jsdoc': [
'error',
{
contexts: ['MethodDefinition:not([accessibility="private"])'],
require: {
ClassDeclaration: true,
},
checkConstructors: false,
enableFixer: false,
},
],
'@typescript-eslint/no-this-alias': [
'error',
{
allowDestructuring: true,
allowedNames: ['node'],
},
],
},
overrides: [
{
Expand Down
10 changes: 0 additions & 10 deletions packages/sdk/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
"test:bench": "vitest bench",
"test:ci": "vitest run --coverage",
"test:yorkie.dev": "TEST_RPC_ADDR=https://api.yorkie.dev vitest run --coverage",
"lint": "eslint . --fix --max-warnings=0 --ext .ts",
"prepare": "pnpm build"
},
"engines": {
Expand Down Expand Up @@ -53,11 +52,7 @@
"@typescript-eslint/parser": "^6.21.0",
"@vitest/coverage-istanbul": "^0.34.5",
"@vitest/coverage-v8": "^0.34.5",
"eslint": "^8.19.0",
"eslint-plugin-jsdoc": "^39.3.3",
"eslint-plugin-prettier": "^4.2.1",
"eslint-plugin-tsdoc": "^0.2.16",
"husky": "^8.0.3",
"prettier": "^2.7.1",
"ts-node": "^10.9.1",
"typedoc": "^0.25.13",
Expand All @@ -74,10 +69,5 @@
"@connectrpc/connect": "^1.4.0",
"@connectrpc/connect-web": "^1.4.0",
"long": "^5.2.0"
},
"husky": {
"hooks": {
"pre-commit": "pnpm lint"
}
}
}
4 changes: 2 additions & 2 deletions packages/sdk/src/document/crdt/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1371,8 +1371,8 @@ export class CRDTTree extends CRDTElement implements GCParent {
const treePos = node.isText
? { node, offset: 0 }
: parentNode && leftChildNode
? this.toTreePos(parentNode, leftChildNode)
: null;
? this.toTreePos(parentNode, leftChildNode)
: null;

if (treePos) {
index = this.indexTree.indexOf(treePos);
Expand Down
Loading
Loading