Adding "Compact Constants" Option #1242
Conversation
* Adding `CompactConstantDefinitions` boolean flag to `tables/tables.go`. * Can be set via tables json file (`--add_tables` / `--tables` command line or `tables` / `addTables` json config option). * Updating `compactStmt()` in `build/print.go`. * If `CompactConstantDefinitions` is true and both statements are assignments (for const definitions), then the extra line should be removed. * Lower precedence than various other checks (like comment checks) to still allow extra lines when necessary. * Updating `setFlags()` in `build/print_test.go`. * Checking golden filenames for ".compactconst." to set `CompactConstantDefinitions` to true. * Adding `CompactConstantDefinitions` reset back to false in returned (deferred) func. * Copying various golden files (which contain constant definitions) to be ".compactconst." files. * Removing extra lines between const definitions as applicable. Fixes bazelbuild#108.
CauhxMilloy
requested review from
larsrc-google,
pmbethe09 and
vladmos
as code owners
|
Hi, I'm not sure if I'm missing any step to get the review started? If there's something I missed, or if there's any desired changes, please let me know. Thanks! |
|
/gemini review |
![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 introduces a CompactConstantDefinitions option to control the formatting of constant assignments, which is a nice enhancement for formatting preferences. The implementation is clean and follows existing patterns in the codebase. The changes are also well-tested with new golden files. I have one suggestion to improve the naming of function parameters in tables/tables.go to avoid confusion with package-level variables, which I've detailed in a specific comment.
|
|
||
| // MergeTables allows a user of the build package to override the special-case rules. The user-provided tables are merged into the built-in tables. | ||
| func MergeTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool) { | ||
| func MergeTables(labelArg, denylist, listArg, sortableListArg, sortDenylist, sortAllowlist map[string]bool, namePriority map[string]int, compactConstantDefinitions, stripLabelLeadingSlashes, shortenAbsoluteLabelsToRelative bool) { |
There was a problem hiding this comment.
The parameter compactConstantDefinitions (and similarly stripLabelLeadingSlashes and shortenAbsoluteLabelsToRelative) shadows the package-level variable of the same name. This makes assignments in this function (e.g., CompactConstantDefinitions = compactConstantDefinitions || CompactConstantDefinitions) and in OverrideTables confusing to read. To improve readability and maintainability, consider renaming these boolean parameters to distinguish them from the package-level variables, for example by prefixing them with new.
2 participants
CompactConstantDefinitionsboolean flag totables/tables.go.--add_tables/--tablescommand line ortables/addTablesjson config option).falseto be backwards-compatible with existing formatted files.compactStmt()inbuild/print.go.CompactConstantDefinitionsis true and both statements are assignments (for const definitions), then the extra line should be removed.setFlags()inbuild/print_test.go.CompactConstantDefinitionsto true.CompactConstantDefinitionsreset back to false in returned (deferred) func.Fixes #108.