feat(internal/postprocessing): add replace and replaceRegex functions#6412
feat(internal/postprocessing): add replace and replaceRegex functions#6412yangyzs wants to merge 3 commits into
Conversation
Add the replace and replaceRegex functions to the postprocessing package. They perform exact string and regex replacements in files and return an error if the target text is not found. Fixes googleapis#6297
![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 two new utility functions, Replace and ReplaceRegex, to the postprocessing package for finding and replacing text in files, along with comprehensive unit tests. The review feedback suggests optimizing both functions by performing search and replacement operations directly on []byte slices using the bytes package and regexp.Regexp.ReplaceAll instead of converting the file contents to strings, which avoids unnecessary allocations and copies.
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "os" | ||
| "regexp" | ||
| "strings" | ||
|
|
||
| "github.com/googleapis/librarian/internal/filesystem" | ||
| ) |
There was a problem hiding this comment.
To support efficient byte-level operations without converting file contents to strings, we should import the bytes package.
| import ( | |
| "errors" | |
| "fmt" | |
| "os" | |
| "regexp" | |
| "strings" | |
| "github.com/googleapis/librarian/internal/filesystem" | |
| ) | |
| import ( | |
| "bytes" | |
| "errors" | |
| "fmt" | |
| "os" | |
| "regexp" | |
| "strings" | |
| "github.com/googleapis/librarian/internal/filesystem" | |
| ) |
| t.Fatal(err) | ||
| } | ||
| err := Replace(path, "Apple", "Go") | ||
| if !errors.Is(err, ErrTextNotFound) { |
There was a problem hiding this comment.
I went with errors.Is and a sentinel error here to align with the error testing guidelines. Let me know if you'd prefer the simpler strings.Contains approach instead.
Address review feedback to use []byte operations instead of string conversions to avoid unnecessary memory allocations.
| // It returns an error if the target file does not exist or if the pattern matches no text. | ||
| func ReplaceRegex(path, pattern, replacement string) error { | ||
| if !strings.HasPrefix(pattern, "(?") { | ||
| pattern = "(?m)" + pattern |
There was a problem hiding this comment.
Why do we need this step?
There was a problem hiding this comment.
s.replace(...) in synthtool uses re.MULTILINE by default (link). It makes file replacements search line-by-line.
In Go, (?m) ensures it works the exact same way, allowing ine-by-line regex in librarian.yaml. Otherwise ^ only matches the very beginning of the whole file.
There was a problem hiding this comment.
Got it, could you add a comment in the code?
| original := "World" | ||
| replacement := "Go" | ||
| want := "Hello Go" | ||
|
|
There was a problem hiding this comment.
nit: remove this empty line.
| } | ||
| } | ||
|
|
||
| func TestReplaceRegex(t *testing.T) { |
There was a problem hiding this comment.
I think this test is a good candidate for parallelization: https://github.com/googleapis/librarian/blob/main/doc/howwewritego.md#running-tests-in-parallel
2 participants
Add the replace and replaceRegex functions to the postprocessing package. They perform exact string and regex replacements in files and return an error if the target text is not found.
Fixes #6297