feat: remove unnecessary keep comments in lists#2232
Merged
jayconrod merged 4 commits intobazel-contrib:masterfrom Nov 25, 2025
Merged
feat: remove unnecessary keep comments in lists#2232jayconrod merged 4 commits intobazel-contrib:masterfrom
jayconrod merged 4 commits intobazel-contrib:masterfrom
Conversation
Contributor
|
Commented in #2228. Let's discuss there before getting into PR details. |
6c919b0 to
d1bd4b4
Compare
Contributor
Author
|
@jayconrod i'm not super happy with the change but maybe it illustrates the files that need to be updated. also removed the ai generated tests for clarity.
|
jayconrod
reviewed
Nov 20, 2025
Contributor
Author
|
@jayconrod fixed according to your suggestions, would appreciate your preference on how to test this (integration/unit tests/none) |
jayconrod
reviewed
Nov 24, 2025
Contributor
jayconrod
left a comment
There was a problem hiding this comment.
For testing, integration tests are best, and for Gazelle they can run quickly. For each test, add a new subdirectory in tests with a MODULE.bazel file and arguments.txt. I think two (nearly identical) tests would be sufficient:
- Unnecessary comments are removed with
fix. Necessary comments are preserved. - Same but with
-remove_noop_keep_comments.
Contributor
|
The current CI failure is unrelated. Should be fixed after merging |
fb4eaf6 to
4e1a3c0
Compare
Contributor
Author
|
@jayconrod updated readme + added integration tests. seems like i revealed that i'm somehow not removing the keep comments for visibility, but maybe can tackle that later and just have this merged? |
jayconrod
approved these changes
Nov 25, 2025
Contributor
jayconrod
left a comment
There was a problem hiding this comment.
Looks good, thank you! Fine to fix the visibility issue in a follow-up.
jayconrod
pushed a commit
that referenced
this pull request
Dec 2, 2025
**What type of PR is this?** Feature **What package or component does this PR mostly affect?** cmd/gazelle **What does this PR do? Why is it needed?** Followup to #2232 to remove keep comments for scalar attribute assignments. **Which issues(s) does this PR fix?** partially addresses #2228, remaining work is to remove keep comments for non-mergeable attributes (which is a little hairy) **Other notes for review** Keeping things abstracted to mergeAttrValues felt more right to me, although the code there to handle the edge cases looks a little wonky. Would appreciate some advice there.
pcj
pushed a commit
to stackb/bazel-gazelle
that referenced
this pull request
Dec 23, 2025
**What type of PR is this?** feature **What package or component does this PR mostly affect?** cmd/gazelle **What does this PR do? Why is it needed?** Removes keep comments in lists when the list item was going to be kept anyways. Allows for automatic cleanup of dependencies that do not have to be manually tracked. **Which issues(s) does this PR fix?** Partially addresses bazel-contrib#2228
pcj
pushed a commit
to stackb/bazel-gazelle
that referenced
this pull request
Dec 23, 2025
**What type of PR is this?** Feature **What package or component does this PR mostly affect?** cmd/gazelle **What does this PR do? Why is it needed?** Followup to bazel-contrib#2232 to remove keep comments for scalar attribute assignments. **Which issues(s) does this PR fix?** partially addresses bazel-contrib#2228, remaining work is to remove keep comments for non-mergeable attributes (which is a little hairy) **Other notes for review** Keeping things abstracted to mergeAttrValues felt more right to me, although the code there to handle the edge cases looks a little wonky. Would appreciate some advice there.
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
feature
What package or component does this PR mostly affect?
cmd/gazelle
What does this PR do? Why is it needed?
Removes keep comments in lists when the list item was going to be kept anyways. Allows for automatic cleanup of dependencies that do not have to be manually tracked.
Which issues(s) does this PR fix?
Partially addresses #2228