Skip to content

chore(linter): ignore file changes in packages directory - #9302

Open
shivanee-p wants to merge 3 commits into
mainfrom
shivaneep-linter-handwritten
Open

chore(linter): ignore file changes in packages directory#9302
shivanee-p wants to merge 3 commits into
mainfrom
shivaneep-linter-handwritten

Conversation

@shivanee-p

@shivanee-p shivanee-p commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Updates the monorepo linter to ignore changes in the top-level packages/ directory containing generated client libraries

Fixes b/560350445 🦕

@shivanee-p
shivanee-p requested a review from a team as a code owner September 12, 2026 00:19
@shivanee-p
shivanee-p requested review from quirogas and removed request for a team September 12, 2026 00:19
@github-actions
github-actions Bot requested a review from feywind September 12, 2026 00:19
@shivanee-p shivanee-p changed the title fix(linter): ignore file changes in packages directory chore(linter): ignore file changes in packages directory Sep 12, 2026
@shivanee-p
shivanee-p removed the request for review from feywind September 12, 2026 00:20

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the linter configuration and script to ignore files inside top-level directories like packages/ and optimizes path segment checks by using Set lookups. Feedback on the changes highlights a bug where running the linter from a subdirectory causes incorrect path resolution; resolving file paths relative to the repository root instead of the current working directory is recommended to ensure correct behavior.

Comment thread bin/linter.mjs
@shivanee-p
shivanee-p force-pushed the shivaneep-linter-handwritten branch from 83cc487 to 29be9f8 Compare September 12, 2026 00:24
@shivanee-p

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the linter script (bin/linter.mjs) and .eslintrc.json to support running from subdirectories by dynamically resolving the repository root path via Git. It also introduces a mechanism to ignore top-level directories like packages/ and optimizes ignored path checks using a Set. Feedback was provided to avoid hardcoding the path to the TypeScript compiler inside node_modules and instead resolve it dynamically using require.resolve for better robustness.

Comment thread bin/linter.mjs Outdated
Comment on lines +396 to +397
await execFileAsync('node', [
'node_modules/typescript/bin/tsc',
path.join(getRepoRoot(), 'node_modules/typescript/bin/tsc'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

According to the general rules, we should avoid hardcoding relative paths to dependencies in node_modules (e.g., 'node_modules/typescript/bin/tsc'). Instead, resolve the path dynamically using require.resolve to ensure robustness across different environments and directory structures. Since this is an ES module, we can dynamically import createRequire from the module package to obtain a require function.

Suggested change
await execFileAsync('node', [
'node_modules/typescript/bin/tsc',
path.join(getRepoRoot(), 'node_modules/typescript/bin/tsc'),
const require = (await import('module')).createRequire(import.meta.url);
await execFileAsync('node', [
require.resolve('typescript/bin/tsc'),
References
  1. Avoid hardcoding relative paths to dependencies in node_modules. Instead, resolve the path dynamically using require.resolve and path.join to ensure robustness across different environments and directory structures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant