You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
OK. The schema for bazel_dep doesn't have repo_name, but let me check if there's any bazel_dep type that might want patches AND repo_name. Looking at the schema, the bazel_dep type only allows type, name, freeze_version, and patches — no repo_name. So the manual construction in gen_bazel_dep doesn't need to handle repo_name.
Now let me think about whether there are any real issues:
Code duplication vs format_bazel_dep_with_override: The new code in gen_bazel_dep manually constructs the template format call instead of using the existing format_bazel_dep_with_override helper. This is a minor concern since the bazel_dep type is simpler (no build_file, no repo_mapping, etc.), but it could lead to inconsistencies if the template evolves.
The version attribute on single_version_override: Bazel's single_version_override accepts an optional version parameter that pins the dependency to a specific version. The new code doesn't include version in the override attrs, which is correct — the version is already specified in the bazel_dep() call. The single_version_override is primarily used here for applying patches, not for version pinning (the bazel_dep already has the version).
Correctness: The generated output should look like:
Schema consistency: The patches field is already present in other schema types, and the new schema addition follows the same pattern. Good.
One minor issue: The bazel_dep schema definition doesn't include repo_name as an allowed property (it has additionalProperties: false), which is fine and consistent — but I notice that if a downstream user wanted to use both patches and repo_name together for a bazel_dep, they'd need to update the schema. This is not a bug in this PR though.
Looking at this more carefully, the code is correct and clean. The approach of handling patches inline in gen_bazel_dep rather than routing through format_bazel_dep_with_override makes sense because:
bazel_dep type repos don't have build_file, repo_mapping, etc.
repo_attributes() would extract patches and add patch_strip, but also try to copy other fields that don't exist
The existing format_bazel_dep_with_override calls repo_attributes() which is designed for the heavier dependency types
The change is small, well-scoped, and correct. The docstring update is appropriate. The schema update matches the code.
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
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.
we do this downstream