chore: change deleted package flag updater to put one per line #3368
Conversation
Summary of ChangesHello @rickeylev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses the challenge of managing excessively long Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the .bazelrc and .pre-commit-config.yaml files to update how deleted packages are handled. It changes the .bazelrc file to list deleted packages on separate lines using the common --deleted_packages format and updates the .pre-commit-config.yaml to use the tools/update_deleted_packages.sh script directly. The update_deleted_packages.sh script is also modified to find Bazel sub-workspaces and mark them as ignored by the root level Bazel project. The changes improve the readability and maintainability of the deleted packages list and simplify the update process.
| echo "$end_marker" >> "$tmpfile" | ||
|
|
||
| # Write the content after the end marker | ||
| sed "1,/$end_marker/d" "$BAZELRC" >> "$tmpfile" |
There was a problem hiding this comment.
The sed command uses 1,/$end_marker/d to delete lines from the beginning of the file up to and including the end marker. This could be problematic if the end marker is not found, as it would delete the entire file. Consider adding a check to ensure that the end marker exists before running this command, or use a more robust approach that doesn't rely on deleting lines based on a marker.
| # GENERATED_DELETED_PACKAGES_START | ||
| common --deleted_packages=examples/build_file_generation |
There was a problem hiding this comment.
The comment # GENERATED_DELETED_PACKAGES_START should be placed on the line immediately before the first common --deleted_packages entry to clearly demarcate the start of the generated section.
| common --deleted_packages=tests/modules/other/with_external_data | ||
| # GENERATED_DELETED_PACKAGES_END |
There was a problem hiding this comment.
The comment # GENERATED_DELETED_PACKAGES_END should be placed on the line immediately after the last common --deleted_packages entry to clearly demarcate the end of the generated section.
| find . -mindepth 2 \( -name WORKSPACE -o -name MODULE.bazel \) | | ||
| while read -r marker; do | ||
| workspace_dir="$(dirname "$marker")" | ||
| echo "$workspace_dir" | ||
| find "$workspace_dir" \( -name BUILD -o -name BUILD.bazel \) -exec dirname {} \; | ||
| done | ||
| ) | sed 's#^\./##' |
There was a problem hiding this comment.
The find command within the found_packages function could be simplified by combining the -name options into a single expression using -o (OR operator) and grouping them with parentheses. This improves readability and potentially performance.
Also, the sed command at the end of the function can be simplified by using the s#^ ./## pattern instead of s#^./## to remove the leading ./ from the paths.
| find . -mindepth 2 \( -name WORKSPACE -o -name MODULE.bazel \) | | |
| while read -r marker; do | |
| workspace_dir="$(dirname "$marker")" | |
| echo "$workspace_dir" | |
| find "$workspace_dir" \( -name BUILD -o -name BUILD.bazel \) -exec dirname {} \; | |
| done | |
| ) | sed 's#^\./##' | |
| find . -mindepth 2 \( -name WORKSPACE -o -name MODULE.bazel \) | | |
| while read -r marker; do | |
| workspace_dir="$(dirname "$marker")" | |
| echo "$workspace_dir" | |
| find "$workspace_dir" \( -name BUILD -o -name BUILD.bazel \) -exec dirname {} \; | |
| done | |
| ) | sed 's#^./##' |
| tmpfile=$(mktemp) | ||
|
|
||
| # Write the content before the start marker | ||
| sed "/$start_marker/q" "$BAZELRC" > "$tmpfile" |
There was a problem hiding this comment.
The sed command uses the q option to quit after finding the start marker. This is efficient, but it might be more robust to use a different approach that ensures the entire file is processed, especially if the start marker is not found. Consider using a different sed command that doesn't rely on quitting mid-file.
| for pkg in $packages; do | ||
| echo "common --deleted_packages=$pkg" >> "$tmpfile" | ||
| done |
There was a problem hiding this comment.
Using a while loop to iterate through the packages is less efficient than using printf to generate all the lines at once and then appending them to the file. This reduces the number of write operations and improves performance. Also, consider adding a check to ensure that the package variable is not empty before appending it to the file.
| for pkg in $packages; do | |
| echo "common --deleted_packages=$pkg" >> "$tmpfile" | |
| done | |
| printf "common --deleted_packages=%s\n" "$packages" >> "$tmpfile" | |
| local packages | ||
| # Set LC_COLLATE so that "/" sorts before "_", which makes | ||
| # for slightly nicer ordering with file paths | ||
| packages=$(found_packages | LC_COLLATE=C sort | uniq) |
There was a problem hiding this comment.
The variable packages is assigned the output of found_packages | LC_COLLATE=C sort | uniq. If found_packages returns an error, this error will not be propagated because it's part of a pipeline. To ensure that errors are caught, it's better to store the output of found_packages in a temporary variable and check its exit code before proceeding.
Also, consider using tr '\n' ' ' instead of a loop to concatenate the packages into a single string, as it's more efficient.
|
It seems like this added the entire gazelle sub-tree, which wasn't all listed before. I think that's the correct thing to do, but I'm confused how things were working previously if those files weren't ignored. Shouldn't In any case, it looks like an exception for |
1 participant
The
--deleted_packagelines are so long they can't be manually updated.They were only put onto a single line because older Bazel versions didn't
support accumulative behavior.
Along the way, switch to using
commoncommand for them, too. The supportedBazel versions are new enough to support it.