Allow callers to explicitly provide types for set and add commands#1448
Allow callers to explicitly provide types for set and add commands#1448vladmos wants to merge 1 commit intobazelbuild:mainfrom
Conversation
Summary of ChangesHello @vladmos, 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 enhances Buildozer's attribute manipulation commands by providing a mechanism for users to explicitly define the type of an attribute's value. This addresses issues where Buildozer's automatic type inference might lead to incorrect formatting or interpretation of values, particularly for Highlights
Changelog
Activity
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
|
There was a problem hiding this comment.
Code Review
The pull request introduces explicit type hints for set and add commands, allowing callers to specify how attribute values should be interpreted. This is a valuable enhancement for controlling attribute formatting, especially when the automatic type inference might be incorrect. The new AttrType enum and parseAttr function provide a clean way to handle these explicit types. New test cases have been added to buildozer/buildozer_test.sh to cover the new functionality, which is good. However, there is a minor issue in the getAttrValueExpr function where a redundant and unreachable code block exists for rawListAttr type handling.
| case attrType == rawListAttr: | ||
| // list of arbitrary objects (idents, ints, already quoted strings, etc.) | ||
| var list []build.Expr | ||
| for _, arg := range args { | ||
| list = append(list, &build.Ident{Name: arg}) | ||
| } | ||
| return &build.ListExpr{List: list} |
There was a problem hiding this comment.
The case attrType == rawListAttr: at line 655 is unreachable. The previous case statement at line 641 already handles attrType == rawListAttr || (attrType == notProvidedTypeAttr && IsIntList(attr)). Since the switch statement executes the first matching case, the block at lines 655-661 will never be reached. This block should be removed to avoid dead code and potential confusion, or the logic should be refactored to ensure correct and distinct handling if different behaviors are intended for rawListAttr under different conditions.
Buildozer tries to guess the format of an attribute's value before adding it to the file, sometimes it does it incorrectly. This PR provides a way to explicitly set a type of the attribute. Currently it's only supported for the
set,set_if_absentandaddcommands, but can be extended further for other commands as well (likely will also be required fordict_setanddict_add).The syntax is backward-compatible with the old command usages:
Attribute names can't contain colons, hence no previosly existing calls should be affected by the change. It should also be straightforward for the callers because resembles the syntax for type hints in Python (and Starlark).