Eslint config upgrade to 10.0.2 #3801
Conversation
WalkthroughThis pull request includes updates to configuration and stylistic elements across several files. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
144-144:❓ Verification inconclusive
ESLint Version Discrepancy and Unauthorized Change Detected
The PR title indicates an upgrade to ESLint version 10.0.2, but the dependency in the
devDependenciessection has been updated to"eslint": "^9.21.0". Please verify whether the intended upgrade is to version 10.0.2 or if the update to 9.21.0 is correct.Additionally, the pipeline failure message (
Unauthorized change/delete detected for file 'package.json') suggests that modifications to this file may require extra approvals or should follow a specific process. Please ensure that the change is authorized according to your project's guidelines.
ESLint Version Mismatch & Authorization Concern
- The PR title suggests an upgrade to ESLint version 10.0.2, yet
package.json(line 144) still specifies"eslint": "^9.21.0". Please confirm whether the intended upgrade is to version 10.0.2 or if retaining version 9.21.0 is correct.- The pipeline error (
Unauthorized change/delete detected for file 'package.json') indicates that modifications to this file may require special approvals or follow a specific process. Verify that this change complies with the project’s guidelines or provide the necessary authorizations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
package.json(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
package.json
[error] 1-1: Unauthorized change/delete detected for file 'package.json'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.eslintignore(0 hunks).eslintrc.json(0 hunks)eslint.config.js(1 hunks)fix-readme-links.js(1 hunks)package.json(1 hunks)scripts/custom-test-env.ts(1 hunks)
💤 Files with no reviewable changes (2)
- .eslintignore
- .eslintrc.json
✅ Files skipped from review due to trivial changes (1)
- fix-readme-links.js
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (12)
scripts/custom-test-env.ts (1)
9-9: Explicit Promise return type addedThis change improves type clarity by explicitly specifying that the async function returns a Promise.
eslint.config.js (11)
1-12: Well-structured imports for ESLint configurationThe imports are well-organized, importing all necessary plugins and parsers for the new flat ESLint configuration format introduced in ESLint v8.21.0 and standardized in v9.0.0.
13-16: Proper setup of ESLint flat configurationThe configuration uses the new flat config format correctly, extending the recommended JS configuration and integrating with Prettier.
17-31: Comprehensive language options configurationThe language options are well-configured with appropriate ECMAScript version, module type, and parser options for TypeScript and JSX.
33-45: Complete plugin setup with React version detectionGood implementation of all necessary plugins with appropriate React version detection in settings.
62-78: Comprehensive TypeScript type checking rulesStrong enforcement of TypeScript best practices including non-null assertions, type assertions, and function return types.
79-121: Detailed naming convention rulesThe naming conventions are very specific and enforce good practices for different entity types:
- PascalCase with specific prefixes for interfaces
- PascalCase for types and enums
- Camel case for variables and parameters
- Leading underscore requirement for private members
This level of specificity will help maintain code consistency.
122-131: JSX-specific styling rules with Prettier integrationThe React JSX rules enforce proper casing and spacing, with Prettier integration for consistent formatting.
132-144: Comprehensive ignore listThe ignore list replaces the removed
.eslintignorefile and appropriately excludes style files, JSON, SVG, and documentation.
145-158: Special handling for GraphQL filesAppropriate specialized rules for GraphQL files with certain TypeScript rules disabled where they don't apply.
159-164: JavaScript-specific rule modificationsAppropriate relaxation of TypeScript-specific rules for JavaScript files.
1-165:❓ Verification inconclusive
Verify ESLint version compatibility with new configuration
The new flat configuration format requires ESLint v8.21.0+ and is standardized in v9.0.0+. According to the PR description, you're upgrading to 10.0.2, which is compatible.
Please confirm that the package.json updates align with this configuration:
🏁 Script executed:
#!/bin/bash # Check ESLint version in package.json grep -A 1 "eslint\":" package.json || grep -A 1 "eslint\": " package.json # Check eslint-config-prettier version grep -A 1 "eslint-config-prettier" package.jsonLength of output: 229
Confirm ESLint Version Alignment
The flat configuration requires ESLint v8.21.0+ (and is standardized from v9.0.0), and the package.json currently specifies"eslint": "^9.21.0", which meets these requirements. However, the PR description mentions upgrading to ESLint 10.0.2. Please verify whether this discrepancy is intentional:
- If the intention is to upgrade to ESLint 10.0.2, update the version in package.json accordingly.
- Otherwise, adjust the PR description to reflect that ESLint v9.21.0 is being used.
The eslint-config-prettier dependency (
"eslint-config-prettier": "^10.1.1") is correctly updated.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3801 +/- ##
====================================================
- Coverage 86.59% 86.39% -0.20%
====================================================
Files 373 361 -12
Lines 9206 9027 -179
Branches 1940 1905 -35
====================================================
- Hits 7972 7799 -173
+ Misses 866 864 -2
+ Partials 368 364 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eslint.config.js (1)
17-31: Consider enforcing or warning on console usage.
Currently,consoleis declared as read-only, but there is no ESLint rule to prevent or warn on calls likeconsole.log(). If your project’s conventions discourage console usage in production, consider enabling a rule such as"no-console": "warn"or"no-console": "error".rules: { + "no-console": "warn", ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
eslint.config.js(1 hunks)schema.graphql(1 hunks)scripts/custom-test-env.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- scripts/custom-test-env.js
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
schema.graphql
[error] 1-1: Unauthorized change/delete detected for file 'schema.graphql'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
eslint.config.js (3)
3-12: Adoption of an extended ESLint ecosystem looks good.
No issues with the chosen plugins (@eslint/js,@typescript-eslint,prettier,react,tsdoc,graphql, etc.). This setup is consistent with a modern TypeScript + React + GraphQL codebase.
56-141: Verify that stricter TypeScript and React rules aren’t breaking existing code.
This block enables rules like@typescript-eslint/no-explicit-any: error,@typescript-eslint/no-unsafe-function-type: error,react/no-multi-comp: error, etc. In particular, disallowinganytypes may break code that relies on them for quick prototyping or 3rd-party libs. A similar concern was raised in a past review.Please run or update the following script to search for “as any” or other
anyusages that might now fail linting:#!/bin/bash # Searching for 'as any' usage in .ts and .tsx files fd -e ts -e tsx --exclude node_modules | xargs rg "as any"
172-179: Good approach to relax TypeScript rules for plain JavaScript files.
Disabling@typescript-eslint/explicit-module-boundary-typesfor.js/.cjs/.mjsaligns well with JavaScript usage, preventing false positives in non-TypeScript files.
|
@noman2002 @palisadoes I think it was completed in the recent PR. Could you please take a look at it? |
|
@palisadoes SIr can you review it once |
|
Please don't ask for a review if tests are failing. |
|
@palisadoes already resolved in PR #3808 |
|
We have to upgrade to v10.0.2 |
Got it, thanks for clearing up my confusion. He's working on eslint-config-prettier; I initially thought he was working on eslint which is v9.22 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/setup/fix-readme-links.spec.ts (2)
28-37: Consider importing the actual implementation instead of redefining it.The
replaceLinksfunction is defined within the test case itself rather than importing the actual implementation fromfix-readme-links.js. This approach tests your test implementation, not the actual code.- const replaceLinks = (filePath: string) => { - let content = fs.readFileSync(filePath, 'utf8') as string; - content = content.replace( - /\[.*?\]\((.*?)README\.md\)/g, - '[Admin Docs](/)', - ); - fs.writeFileSync(filePath, content, 'utf8'); - }; + import { replaceLinks } from '../fix-readme-links.js';Additionally, the regex pattern doesn't preserve path information before README.md, which might be an issue if links point to README.md files in different directories.
21-47: Add test for error handling.The current test only covers the happy path. Consider adding tests for error scenarios like file not found or permission issues.
it('should handle file read errors', () => { vi.spyOn(fs, 'readFileSync').mockImplementation(() => { throw new Error('File not found'); }); expect(() => replaceLinks(filePath)).toThrow('File not found'); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/setup/fix-readme-links.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (5)
src/setup/fix-readme-links.spec.ts (5)
1-6: Good use of Vitest testing utilities and mocking.The imports are properly set up with appropriate test utilities from Vitest. The
fsmodule is correctly mocked to prevent actual filesystem operations during tests.
9-19: Test data is well structured.The mock content and expected output are clearly defined, making the test intentions easy to understand.
21-27: Good testing practices with beforeEach.Proper test setup with mock restoration and spying on filesystem functions.
38-46: Good test assertions.The test properly verifies both reading and writing operations with the correct parameters.
7-8:✅ Verification successful
Verify the file path structure.
The path includes 'docs' twice (
path.join('docs', 'docs', 'auto-docs', 'test.md')). Please verify this matches your project's directory structure.
🏁 Script executed:
#!/bin/bash # Check if this path structure exists in the repository fd --type directory --max-depth 3 docsLength of output: 94
File Path Verification Passed – No Changes Required
The directory structure in the repository confirms that the double "docs" folder is intentional. The shell script output shows that the pathdocs/docs/auto-docs/exists, so the current code usingconst filePath = path.join('docs', 'docs', 'auto-docs', 'test.md');accurately reflects the project's layout.
|
I'm going to close this. Please resubmit.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/setup/fix-readme-links.spec.ts (6)
10-17: Consider improving the regex pattern and adding error handling.The current regex pattern might not capture all valid Markdown link formats. Consider using a more robust pattern that accounts for various Markdown link syntaxes. Additionally, the function lacks error handling for file operations, which could lead to uncaught exceptions in production.
const replaceLinks = (filePath: string) => { + try { let content = fs.readFileSync(filePath, 'utf8') as string; - content = content.replace(/\[.*?\]\((.*?)README\.md\)/g, () => { + content = content.replace(/\[(.*?)\]\(([^)]*README\.md[^)]*)\)/g, () => { return '[Admin Docs](/)'; }); fs.writeFileSync(filePath, content, 'utf8'); return content; + } catch (error) { + console.error(`Error processing file ${filePath}:`, error); + throw error; + } };
24-32: Verify that writeFileSync is called with the correct arguments.The test confirms the return value but doesn't verify that
writeFileSyncis called with the transformed content. This would ensure the file is actually being written correctly.it('should replace README.md links with Admin Docs link', () => { // Test case 1: Basic replacement const mockContent1 = '[Some Link](./README.md)'; vi.spyOn(fs, 'readFileSync').mockReturnValue(mockContent1); - vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); + const writeFileSpy = vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); const result = replaceLinks(filePath); expect(result).toBe('[Admin Docs](/)'); + expect(writeFileSpy).toHaveBeenCalledWith(filePath, '[Admin Docs](/)', 'utf8'); });
20-22: Move common mock setup to beforeEach.The
writeFileSyncmock implementation is repeated in each test. Consider moving it to thebeforeEachblock to reduce code duplication.beforeEach(() => { vi.restoreAllMocks(); + vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); });
34-42: Add verification of writeFileSync call.Similar to the first test, verify that
writeFileSyncis called with the correct transformed content.it('should handle multiple README.md links', () => { // Test case 2: Multiple replacements const mockContent2 = '[Link1](README.md)[Link2](./docs/README.md)'; vi.spyOn(fs, 'readFileSync').mockReturnValue(mockContent2); - vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); + const writeFileSpy = vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); const result = replaceLinks(filePath); expect(result).toBe('[Admin Docs](/)[Admin Docs](/)'); + expect(writeFileSpy).toHaveBeenCalledWith(filePath, '[Admin Docs](/)[Admin Docs](/)', 'utf8'); });
44-53: Add verification of writeFileSync call.As with the previous tests, add verification that
writeFileSyncis called with the correct arguments.it('should handle content with no README.md links', () => { // Test case 3: No replacements needed const mockContent3 = 'No links here'; vi.spyOn(fs, 'readFileSync').mockReturnValue(mockContent3); - vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); + const writeFileSpy = vi.spyOn(fs, 'writeFileSync').mockImplementation(() => {}); const result = replaceLinks(filePath); expect(result).toBe('No links here'); + expect(writeFileSpy).toHaveBeenCalledWith(filePath, 'No links here', 'utf8'); });
19-53: Consider adding test cases for error handling.The test suite would be more robust if it included tests for error scenarios, such as file read/write failures.
it('should handle file read errors', () => { vi.spyOn(fs, 'readFileSync').mockImplementation(() => { throw new Error('File read error'); }); expect(() => replaceLinks(filePath)).toThrow('File read error'); }); it('should handle file write errors', () => { vi.spyOn(fs, 'readFileSync').mockReturnValue('Some content'); vi.spyOn(fs, 'writeFileSync').mockImplementation(() => { throw new Error('File write error'); }); expect(() => replaceLinks(filePath)).toThrow('File write error'); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/setup/fix-readme-links.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (1)
src/setup/fix-readme-links.spec.ts (1)
1-8: LGTM! Proper testing setup.Good job setting up the test environment with Vitest and mocking the filesystem to avoid actual I/O operations during tests.
Updating eslint_config_upgrade from 9.1.0 from 10.0.2 .
Issue: #3774
Summary by CodeRabbit