Conversation
…s/google-cloud-node into copyright-year-changes
There was a problem hiding this comment.
Code Review
This pull request introduces a script, bin/check-package-years.sh, to validate and ensure consistency of copyright years across package files, along with updates to various files to reflect the 2026 copyright year. The review identified that the find command in the script was overly restrictive, excluding files that it was intended to check, and suggested using process substitution to improve the robustness of the file processing loop.
| \( -name "*.ts" -o -name "*.js" \) \ | ||
| -not -path "*/node_modules/*" \ | ||
| -not -path "*/.git/*" \ | ||
| -not -path "*/dist/*" \ | ||
| -not -path "*/build/*" \ | ||
| -not -name "LICENSE" \ | ||
| -not -name "CHANGELOG.md" \ | ||
| -not -name "package.json" \ | ||
| -not -name "package-lock.json" \ | ||
| -not -name "pnpm-lock.yaml" \ | ||
| -not -name "prettier.config.js" \ | ||
| -not -name ".prettierrc.js" \ | ||
| -not -name ".eslintrc.js" \ | ||
| -not -name "webpack.config.js" \ | ||
| -not -name "rollup.config.js" \ |
There was a problem hiding this comment.
The find command has two issues that cause it to not check all relevant files:
- Incomplete file types: It only checks
*.tsand*.jsfiles, but this PR also updates copyright headers in.cjsand.yamlfiles. These should be included for comprehensive checking. - Incorrect exclusions: It excludes several configuration files (e.g.,
.prettierrc.js,webpack.config.js). However, this PR updates copyrights in those very files, which implies they should be checked. These exclusions should be removed.
To ensure the script correctly validates all intended files, please update the find command.
| \( -name "*.ts" -o -name "*.js" \) \ | |
| -not -path "*/node_modules/*" \ | |
| -not -path "*/.git/*" \ | |
| -not -path "*/dist/*" \ | |
| -not -path "*/build/*" \ | |
| -not -name "LICENSE" \ | |
| -not -name "CHANGELOG.md" \ | |
| -not -name "package.json" \ | |
| -not -name "package-lock.json" \ | |
| -not -name "pnpm-lock.yaml" \ | |
| -not -name "prettier.config.js" \ | |
| -not -name ".prettierrc.js" \ | |
| -not -name ".eslintrc.js" \ | |
| -not -name "webpack.config.js" \ | |
| -not -name "rollup.config.js" \ | |
| \( -name "*.ts" -o -name "*.js" -o -name "*.cjs" -o -name "*.yaml" \) \ | |
| -not -path "*/node_modules/*" \ | |
| -not -path "*/.git/*" \ | |
| -not -path "*/dist/*" \ | |
| -not -path "*/build/*" \ | |
| -not -name "LICENSE" \ | |
| -not -name "CHANGELOG.md" \ | |
| -not -name "package.json" \ | |
| -not -name "package-lock.json" \ | |
| -not -name "pnpm-lock.yaml" |
| tmp_file=$(mktemp) | ||
| find "$pkg_path" -type f \ | ||
| \( -name "*.ts" -o -name "*.js" \) \ | ||
| -not -path "*/node_modules/*" \ | ||
| -not -path "*/.git/*" \ | ||
| -not -path "*/dist/*" \ | ||
| -not -path "*/build/*" \ | ||
| -not -name "LICENSE" \ | ||
| -not -name "CHANGELOG.md" \ | ||
| -not -name "package.json" \ | ||
| -not -name "package-lock.json" \ | ||
| -not -name "pnpm-lock.yaml" \ | ||
| -not -name "prettier.config.js" \ | ||
| -not -name ".prettierrc.js" \ | ||
| -not -name ".eslintrc.js" \ | ||
| -not -name "webpack.config.js" \ | ||
| -not -name "rollup.config.js" \ | ||
| > "$tmp_file" | ||
|
|
||
| while IFS= read -r file; do | ||
| if [ -z "$file" ]; then continue; fi | ||
|
|
||
| # Extract the year from the first copyright line found in the file | ||
| # We use grep -i to be case-insensitive and || true to be robust | ||
| year=$(grep -iohE "Copyright [0-9]{4}" "$file" | head -n 1 | awk '{print $2}' || true) | ||
|
|
||
| if [ -z "$year" ]; then | ||
| # If the file is missing a copyright year, report it | ||
| # Note: We only report this if it's a file type we expect to have it | ||
| echo "Error: Missing copyright year in file: $file" | ||
| EXIT_CODE=1 | ||
| continue | ||
| fi | ||
|
|
||
| if [ -z "$first_year" ]; then | ||
| first_year="$year" | ||
| first_file="$file" | ||
| elif [ "$year" != "$first_year" ]; then | ||
| echo "Error: Copyright year mismatch in package: $pkg" | ||
| echo " $first_file: $first_year" | ||
| echo " $file: $year" | ||
| EXIT_CODE=1 | ||
| # We don't break here to allow finding all issues in this package | ||
| fi | ||
| done < "$tmp_file" | ||
| rm "$tmp_file" |
There was a problem hiding this comment.
The use of a temporary file can be avoided here. Using process substitution (< <(...)) is a cleaner and more robust way to pipe the output of find into the while loop. This avoids creating a file on disk and eliminates the need for manual cleanup with rm (line 95), which might not run if the script is interrupted.
This refactoring would look like:
# ...
first_year=""
first_file=""
# Find all files and pipe them to the inner loop
while IFS= read -r file; do
# ... (inner loop logic)
done < <(find "$pkg_path" -type f ...)
# No rm needed
# ...
experiment