fix(librarian): bump command only modifies versions in librarian.yaml #3676
Conversation
First instance of a pattern we may need elsewhere: if we need to write out a new version of librarian.yaml at the end of a command, and the command uses code which needs defaults filling in (as per prepareLibrary) or might make other changes we don't want to persist, clone the config first and only copy the desired changes at the end. Fixes googleapis#3668
Summary of ChangesHello @jskeet, 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 addresses an issue where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 robust pattern for making isolated changes to the librarian.yaml configuration during commands like bump. By cloning the configuration, operating on the copy, and then selectively merging back only the intended changes (library versions), it prevents unintended modifications from being persisted. The implementation is sound and well-tested. I've provided a few suggestions to improve the implementation of copyLibraryVersions for better performance and to align more closely with idiomatic Go practices.
| pristineCfg, err = copyLibraryVersions(cfg, pristineCfg) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This updates the call to copyLibraryVersions to align with the suggested refactoring of that function in a separate comment.
if err := copyLibraryVersions(cfg, pristineCfg); err != nil {
return err
}| // copyLibraryVersions copies the version (and only the version) of | ||
| // every library from dirtyCfg to pristineCfg, so that we can save just | ||
| // the version number changes. The function returns an error if the two | ||
| // configurations do not have the same set of libraries (by name). | ||
| func copyLibraryVersions(dirtyCfg, pristineCfg *config.Config) (*config.Config, error) { | ||
| if len(dirtyCfg.Libraries) != len(pristineCfg.Libraries) { | ||
| return nil, fmt.Errorf("mismatched library count after bump: %d != %d", len(dirtyCfg.Libraries), len(pristineCfg.Libraries)) | ||
| } | ||
| // We don't care about whether or not the libraries are in the same order, | ||
| // so we just find them by name. | ||
| for _, dirtyLibrary := range dirtyCfg.Libraries { | ||
| pristineLibrary, err := libraryByName(pristineCfg, dirtyLibrary.Name) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| pristineLibrary.Version = dirtyLibrary.Version | ||
| } | ||
| return pristineCfg, nil | ||
| } |
There was a problem hiding this comment.
I have two suggestions for improving copyLibraryVersions:
- Idiomatic Signature: The function modifies its
pristineCfgparameter in-place but also returns it. In Go, it's more idiomatic for functions that modify an argument to not return it. I've changed the signature tofunc copyLibraryVersions(from, to *config.Config) errorand renamed parameters for clarity. - Performance: The current implementation has a time complexity of O(N^2) due to calling
libraryByName(which does a linear scan) inside a loop. This can be optimized to O(N) by building a map of the libraries from the destination config first.
The suggested code below incorporates both of these improvements. Note that this change will require updating the call sites in runBump and TestCopyLibraryVersions, for which I have left separate comments.
// copyLibraryVersions copies the version (and only the version) of
// every library from `from` to `to`, so that we can save just
// the version number changes. The function returns an error if the two
// configurations do not have the same set of libraries (by name).
func copyLibraryVersions(from, to *config.Config) error {
if len(from.Libraries) != len(to.Libraries) {
return fmt.Errorf("mismatched library count after bump: %d != %d", len(from.Libraries), len(to.Libraries))
}
toLibraries := make(map[string]*config.Library, len(to.Libraries))
for _, lib := range to.Libraries {
toLibraries[lib.Name] = lib
}
for _, fromLibrary := range from.Libraries {
toLibrary, ok := toLibraries[fromLibrary.Name]
if !ok {
return fmt.Errorf("library %q not found in destination config", fromLibrary.Name)
}
toLibrary.Version = fromLibrary.Version
}
return nil
}References
- The repository style guide (line 14) recommends following idiomatic Go patterns from the Go Code Review Comments wiki. The suggestion to change the function signature to not return a modified input parameter aligns with these idiomatic practices, improving code clarity. (link)
There was a problem hiding this comment.
@julieqiu: the "idiomatic signature" part was actually what I originally had, before remembering https://github.com/googleapis/librarian/blob/main/doc/howwewritego.md#make-mutations-explicit - please check that my understanding is correct.
I'll apply the "use a map" idea now though.
| pristine, err := copyLibraryVersions(dirty, pristine) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
This updates the call to copyLibraryVersions to align with the suggested refactoring of that function in a separate comment.
if err := copyLibraryVersions(dirty, pristine); err != nil {
t.Fatal(err)
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3676 +/- ##
==========================================
- Coverage 82.43% 82.37% -0.06%
==========================================
Files 138 138
Lines 12665 12690 +25
==========================================
+ Hits 10440 10454 +14
- Misses 1735 1741 +6
- Partials 490 495 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
(The coverage is tricky to improve as I'd need to figure out a way of making configuration formatting/parsing fail...) |
This was causing Two reasons to avoid filling in the defaults in (which aren't removed by tidy):
If |
|
@julieqiu As I commented on #3678, I suspect we will want Basically I suspect that everywhere that language-specific code will need to do anything non-trivial (e.g. modify a file), it will benefit from being passed a "normalized" config, where it can know immediately where to find the source code etc. I don't think we want to bake "apply defaults to figure stuff out" code in each language's functions. In fact, even the existing Rust code for |
3 participants
First instance of a pattern we may need elsewhere: if we need to write out a new version of librarian.yaml at the end of a command, and the command uses code which needs defaults filling in (as per prepareLibrary) or might make other changes we don't want to persist, clone the config first and only copy the desired changes at the end.
Fixes #3668