Add support for mapped repository names in use_repo_add

[malt3]

Implement support for three types of repository specifications:

• Simple names: foo -> use_repo(ext, "foo")
• Mapped names with valid identifiers: bar=baz -> use_repo(ext, bar = "baz")
• Invalid identifiers use dict unpacking: foo.2=foo -> use_repo(ext, **{"foo.2": "foo"})

This follows the Bazel documentation recommendations for handling repository names that are not valid Starlark identifiers by using dictionary unpacking syntax.

[gemini-code-assist]

Summary of Changes

Hello @malt3, I'm Gemini Code Assist¹! 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 significantly enhances the use_repo_add command within Buildozer by introducing support for mapped repository names. This allows users to define custom local names for external repositories, providing greater flexibility and control over Bazel module dependencies. The changes involve updating the command's parsing and generation logic to correctly handle different mapping syntaxes, including those with identifiers that are not valid Python names, and ensuring proper conflict resolution with existing repository declarations.

Highlights

• Enhanced use_repo_add functionality: The use_repo_add command now supports specifying mapped repository names, allowing for custom local names for imported repositories.
• Support for various mapping syntaxes: Repositories can be added using simple names (e.g., foo), mapped names with valid identifiers (e.g., bar=baz), or mapped names with invalid identifiers (e.g., foo.2=foo) which are handled via dictionary unpacking.
• Refactored AddRepoUsages logic: The core logic for adding repository usages has been updated to intelligently parse and apply these new mapping types, including conflict resolution for existing arguments.
• New helper functions: Introduced parseRepoMapping, isValidIdentifier, and addOrExtendDictUnpack to facilitate parsing, identifier validation, and dynamic dictionary manipulation for use_repo calls.
• Comprehensive test coverage: Extensive new test cases have been added to validate the correct behavior of mapped repository names, including edge cases and conflict scenarios.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

The pull request enhances the use_repo_add command in buildozer to support specifying repositories using simple names, valid keyword arguments (e.g., foo=bar), and invalid keyword arguments (e.g., foo.2=bar) which are handled via dictionary unpacking. This involved a significant refactoring of the AddRepoUsages function in bzlmod.go to parse these different formats, manage conflicts by replacing existing mappings, and correctly generate the use_repo call arguments. New helper functions (parseRepoMapping, isValidIdentifier, addOrExtendDictUnpack) were introduced, and the README.md was updated to document these new capabilities. Corresponding unit tests were added to cover these new scenarios, including mixed argument types, extending existing dict unpacking, and conflict resolution. Review comments indicate that the refactored AddRepoUsages function is still too complex and suggest further extraction of logic into smaller helper functions, and also point out redundant fields (isDictUnpacking) and unused variables (positionalValuesToAdd) that should be removed.

[malt3]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces support for mapped repository names in use_repo_add, a valuable enhancement for buildozer. The implementation is well-structured, breaking down the logic into several helper functions, and includes comprehensive tests and documentation updates. My review identifies a couple of critical issues that could lead to a panic, a high-severity issue where invalid Starlark code could be generated, and a few medium-severity suggestions for improving code clarity, robustness against malformed input, and efficiency.

[malt3]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces valuable support for mapped repository names in use_repo_add, which is a significant improvement. The implementation is well-designed, breaking down the complex logic of AddRepoUsages into smaller, understandable helper functions. The handling of simple names, mapped names with valid identifiers, and mapped names with invalid identifiers (using dictionary unpacking) is comprehensive. The logic for removing conflicting repository definitions before adding new ones is also well-executed. The test coverage is extensive, and the documentation updates are clear and helpful. I have one suggestion to enhance the validation of Starlark identifiers.

[malt3]

/gemini review

[gemini-code-assist]

Code Review

This pull request adds support for mapped repository names in use_repo_add, including simple names, mapped names with valid identifiers, and mapped names with invalid identifiers using dictionary unpacking. The implementation is well-structured, breaking down the logic into smaller, manageable functions. The accompanying tests are comprehensive, covering a wide range of scenarios. I've added a couple of suggestions to improve input validation for robustness. Overall, this is a great enhancement.

[malt3]

@oreflow gentle ping. This could unblock a change in Bazel (see bazelbuild/bazel#27891).

[oreflow]

Starting off with high level topics before going into code details.

**kwargs support bar=baz -> use_repo(ext, bar = "baz")

This part of the change looks good overall, but there's an implicit assumption added that if calling use_repo_add with an existing key, the argument is replaced, which is inconsistent with other buildozer behaviors as add and dict_add both state that "If the key was already present, it will not be overwritten." (see difference between dict_set and dict_add).

foo.2=foo -> use_repo(ext, **{"foo.2": "foo"})

This part of the change adds a fair bit of complexity in parsing and validation, which also in their implementation require embedding part of the Starlark language specification in this repo. Just as an example, the list of reserved Bazel keywords does not seem to match https://github.com/bazelbuild/starlark/blob/master/spec.md#built-in-constants-and-functions, and it would be infeasible for us to maintain this in the buildtools repo as the starlark language evolves. So if this is kept we should look into using some authorative source to import on this rather than hardcoding it in this codebase.

Do you have a specific use case in mind where you would need to add repos with names matching matches reserved keywords? It seems a bit like this should just be discouraged, even if it would be possible using dict unpacking it seems better if this would return an error.

Additionally behaviors of Buildozer are generally not trying to be smart, so the conditional conversion of reserved / invalid identifiers into dict unpacking might be confusing to users. It might be simpler to place this in the hands of the users, e.g. if a user really wants to add dict-unpacked-values with use_repo_add, it might be more intuitive to support a syntax like use_repo_add {"foo": "bar"}, rather than doing a conversion based on logic here.

[oreflow]

Just mentioning it since I noticed this first pass and might miss it if revisiting, I believe byte index here will not be accurate for a string value with unicode characters, as they need to be digested as runes.

[malt3]

Thank you for taking a look!

This part of the change looks good overall, but there's an implicit assumption added that if calling use_repo_add with an existing key, the argument is replaced, which is inconsistent with other buildozer behaviors [...]

I agree that this is inconsistent. That being said, replacing existing keys is the behavior we want from bazel mod tidy, which is the use case I have in mind. That leaves me with two options: either we add use_repo_set with the current behavior (replace keys if they already exist) and change use_repo_add to leave keys unchanged if they already exist, or we accept this inconsistency and document it clearly. What's your preference?
A third option would be to keep use_repo_add completely unchanged (no support for mapping repo names) and reserve the mapping functionality for use_repo_set.

Do you have a specific use case in mind where you would need to add repos with names matching matches reserved keywords? It seems a bit like this should just be discouraged, even if it would be possible using dict unpacking it seems better if this would return an error.

I was merely trying to support the full set of possible repo names according to Bazel's documentation. I do agree that this introduces a lot of complexity.
Furthermore, I would be fine to drop support for dict unpacking and simply return an error. This slightly limits the connected Bazel feature of mapping repo names via extension metadata, but that limitation is acceptable. Does that work for you?

I'll simplify and adapt this PR based on your feedback.

[oreflow]

I agree that this is inconsistent. That being said, replacing existing keys is the behavior we want from bazel mod tidy, which is the use case I have in mind. That leaves me with two options: either we add use_repo_set with the current behavior (replace keys if they already exist) and change use_repo_add to leave keys unchanged if they already exist, or we accept this inconsistency and document it clearly. What's your preference?

I would go with use_repo_set, just for the consistency, I'd imagine implementation/complexity-wise it could still just go in a single internal function for _add and _set which just takes a boolean whether to replace or ignore existing values.

A third option would be to keep use_repo_add completely unchanged (no support for mapping repo names) and reserve the mapping functionality for use_repo_set.

I believe the mapping addition to use_repo_add makes sense, plus it's nice for _add and _set to support the same feature set. But feel free to omit it if you feel it adds complexity.

Furthermore, I would be fine to drop support for dict unpacking and simply return an error. This slightly limits the connected Bazel feature of mapping repo names via extension metadata, but that limitation is acceptable. Does that work for you?

I think the Error approach is good, however this would still require keeping the parsing and validation, I would even go as far as just allowing users to set mappings with invalid names, as mentioned Buildozer doesn't really deal with validation / opinionated / smart functionality. So if someone explicitly tells Buildozer to do something which will be invalid, I believe we can simply allow them to do so to keep the functionality simple and intuitive, in general Buildozer is more just for the convenience of automated edits, rather than validation.

Sidenote, If we wanted validation it might make sense to add a Buildifier warning for any re-use of reserved keywords / invalid identifiers (IIRC this doesn't exist already). That way it would also support a lot more use cases. But not really tied to this PR

Thanks for following up so actively and working on this :)