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

Fixed ESLint bugs that required ts-specific rules for js functions and added prettier formatting (re-open #3184) #3229

Merged
4 changes: 2 additions & 2 deletions .github/workflows/pull-request.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
##############################################################################

Check warning on line 1 in .github/workflows/pull-request.yml

View workflow job for this annotation

GitHub Actions / Performs linting, formatting, type-checking, checking for different source and target branch

File ignored by default.
##############################################################################
#
# NOTE!
Expand Down Expand Up @@ -66,8 +66,8 @@

- name: Check for localStorage Usage
run: |
chmod +x scripts/githooks/check-localstorage-usage.js
node scripts/githooks/check-localstorage-usage.js --scan-entire-repo
chmod +x scripts/githooks/check-localstorage-usage.ts
tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 18, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Update file permissions handling.

The current approach of using chmod on a TypeScript file is unconventional.

Consider one of these approaches:

  1. Move the shebang to a shell script wrapper
  2. Use npx to execute the TypeScript file directly:
-          chmod +x scripts/githooks/check-localstorage-usage.ts
-          tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo
+          npx tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
chmod +x scripts/githooks/check-localstorage-usage.ts
tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo
npx tsx scripts/githooks/check-localstorage-usage.ts --scan-entire-repo

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

@yugal07 Perfect! The changes have been implemented correctly. The workflow now uses npx tsx directly, which is a cleaner approach.

🔗 Analysis chain

@yugal07 Perfect! The changes have been implemented correctly. The workflow now uses npx tsx directly, which is a cleaner approach.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the current content of the workflow file
rg -A 1 "Check for localStorage Usage" .github/workflows/pull-request.yml

Length of output: 203


If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


- name: Compare translation files
run: |
Expand Down
2 changes: 1 addition & 1 deletion .lintstagedrc.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{

Check warning on line 1 in .lintstagedrc.json

View workflow job for this annotation

GitHub Actions / Performs linting, formatting, type-checking, checking for different source and target branch

File ignored by default.
"**/*.{ts,tsx,yml}": "eslint --fix",
"**/*.{ts,tsx,json,scss,css,yml}": "prettier --write",
"**/*.{ts,tsx}": "node scripts/githooks/check-localstorage-usage.js"
"**/*.{ts,tsx}": "tsx scripts/githooks/check-localstorage-usage.ts"
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 18, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix the pipeline failure by installing tsx.

The pipeline is failing because the tsx package is not installed in the CI environment.

Add tsx to the devDependencies in package.json and ensure it's installed in the CI workflow:

+"tsx": "^4.7.0",

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Author

Choose a reason for hiding this comment

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

I added npx tsx instead, so it doesnot need to have tsx installed in the CI workflow

Copy link
Contributor

Choose a reason for hiding this comment

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

@yugal07 You're absolutely right! Using npx tsx is a better approach as it doesn't require installing tsx as a dependency. The CI environment already has npx available through Node.js installation. I apologize for my earlier suggestion to add it to devDependencies.

(`・ω・´)ゞ

🔗 Analysis chain
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the CI workflow file
cat .github/workflows/pull-request.yml

# Check recent workflow runs for this PR
gh run list --limit 5 --branch "$(git branch --show-current)"

Length of output: 15463

}
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
"update:toc": "node scripts/githooks/update-toc.js",
"lint-staged": "lint-staged --concurrent false",
"setup": "tsx setup.ts",
"check-localstorage": "node scripts/githooks/check-localstorage-usage.js"
"check-localstorage": "tsx scripts/githooks/check-localstorage-usage.ts"
},
"eslintConfig": {
"extends": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,44 +3,49 @@
import { readFileSync, existsSync } from 'fs';
import path from 'path';
import { execSync } from 'child_process';
import type { ExecSyncOptionsWithStringEncoding } from 'child_process';

const args = process.argv.slice(2);
const scanEntireRepo = args.includes('--scan-entire-repo');
const args: string[] = process.argv.slice(2);
const scanEntireRepo: boolean = args.includes('--scan-entire-repo');

const containsSkipComment = (file) => {
const containsSkipComment = (file: string): boolean => {
try {
const content = readFileSync(file, 'utf-8');
return content.includes('// SKIP_LOCALSTORAGE_CHECK');
} catch (error) {
console.error(`Error reading file ${file}:`, error.message);
console.error(
`Error reading file ${file}:`,
error instanceof Error ? error.message : error,
);
return false;
}
};

const getModifiedFiles = () => {
const getModifiedFiles = (): string[] => {
try {
const options: ExecSyncOptionsWithStringEncoding = { encoding: 'utf-8' };

if (scanEntireRepo) {
const result = execSync('git ls-files | grep ".tsx\\?$"', {
encoding: 'utf-8',
});
const result = execSync('git ls-files | grep ".tsx\\?$"', options);
return result.trim().split('\n');
}

const result = execSync('git diff --cached --name-only', {
encoding: 'utf-8',
});
const result = execSync('git diff --cached --name-only', options);
return result.trim().split('\n');
} catch (error) {
console.error('Error fetching modified files:', error.message);
console.error(
'Error fetching modified files:',
error instanceof Error ? error.message : error,
);
process.exit(1);
}
return [];
};

const files = getModifiedFiles();

const filesWithLocalStorage = [];
const files: string[] = getModifiedFiles();
const filesWithLocalStorage: string[] = [];

const checkLocalStorageUsage = (file) => {
const checkLocalStorageUsage = (file: string): void => {
if (!file) {
return;
}
Expand All @@ -49,7 +54,7 @@ const checkLocalStorageUsage = (file) => {

// Skip files with specific names or containing a skip comment
if (
fileName === 'check-localstorage-usage.js' ||
fileName === 'check-localstorage-usage.ts' || // Updated extension
fileName === 'useLocalstorage.test.ts' ||
fileName === 'useLocalstorage.ts' ||
containsSkipComment(file)
Expand All @@ -73,7 +78,10 @@ const checkLocalStorageUsage = (file) => {
console.log(`File ${file} does not exist.`);
}
} catch (error) {
console.error(`Error reading file ${file}:`, error.message);
console.error(
`Error reading file ${file}:`,
error instanceof Error ? error.message : error,
);
}
};

Expand All @@ -86,10 +94,10 @@ if (filesWithLocalStorage.length > 0) {

console.info(
'\x1b[34m%s\x1b[0m',
'\nInfo: Consider using custom hook functions.'
'\nInfo: Consider using custom hook functions.',
);
console.info(
'Please use the getItem, setItem, and removeItem functions provided by the custom hook useLocalStorage.\n'
'Please use the getItem, setItem, and removeItem functions provided by the custom hook useLocalStorage.\n',
);

process.exit(1);
Expand Down
Loading