feat: add basic monorepo linter to enforce GTS standards#8446
feat: add basic monorepo linter to enforce GTS standards#8446shivanee-p wants to merge 10 commits into
Conversation
…ction in linter.mjs
There was a problem hiding this comment.
Code Review
This pull request introduces a new linter script (bin/linter.mjs) that uses Prettier to check formatting on changed files, and updates package.json to run this script during the lint step. The review feedback highlights critical security and robustness improvements: specifically, replacing execSync with execFileSync to prevent command injection vulnerabilities and handle filenames with spaces or special characters safely, and dynamically resolving the base branch using process.env.GITHUB_BASE_REF to support CI environments.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…nd invocation in linter.mjs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new linter script, bin/linter.mjs, which runs Prettier to check formatting on changed files against a base branch, and updates the lint script in package.json to execute it. Feedback on the implementation highlights two issues: potential failures in CI environments if the local base branch is missing (suggesting a fallback to the remote tracking branch), and cross-platform compatibility issues on Windows when executing npx via execFileSync (suggesting dynamically resolving the command to npx.cmd).
| // Define the base branch to compare against | ||
| const baseBranch = process.env.GITHUB_BASE_REF || 'main'; |
There was a problem hiding this comment.
In CI environments (such as GitHub Actions), the local main branch (or the branch specified in GITHUB_BASE_REF) might not exist locally because of shallow clones or because only the PR branch is checked out. However, the remote tracking branch (e.g., origin/main) is typically available. Using only baseBranch directly can cause git diff to fail with fatal: ambiguous argument in CI.
We can make this more robust by checking if the local ref exists, and falling back to the remote tracking branch (origin/${base}) if it does not.
// Define the base branch to compare against, falling back to the remote tracking branch if the local ref is missing (common in CI shallow clones)
const baseBranch = (() => {
const base = process.env.GITHUB_BASE_REF || 'main';
try {
execFileSync('git', ['rev-parse', '--verify', base], {stdio: 'ignore'});
return base;
} catch {
return 'origin/' + base;
}
})();| execFileSync('npx', ['prettier', '--check', ...filesToCheck], { | ||
| stdio: 'inherit', | ||
| }); |
There was a problem hiding this comment.
On Windows, executing npx directly via execFileSync will fail with an ENOENT error because npx is a shell script (npx.cmd) rather than a binary executable. To ensure cross-platform compatibility for developers running the linter locally on Windows, we should resolve the command name dynamically based on the platform.
const npxCmd = process.platform === 'win32' ? 'npx.cmd' : 'npx';
execFileSync(npxCmd, ['prettier', '--check', ...filesToCheck], {
stdio: 'inherit',
});
Add a liner CI check since the gapic-generator now ensures that generated code adheres to the Prettier code style.
The linter only checks formatting for right now, but more checks will be added during the Node.js Fixit Sprint to come up to gts standards. The check only checks files that were touched in the relevant PR to avoid tons of violations as we work to get the monorepo in adherence.
Tested locally via an unformatted file and confirmed the suggested
prettiercommand was in fact correct.