Skip to content

refactor(internal/librarianops/generate): move github functions to its own file#4689

Open
miguelvelezsa wants to merge 4 commits intomainfrom
refactor-librarianops-generate
Open

refactor(internal/librarianops/generate): move github functions to its own file#4689
miguelvelezsa wants to merge 4 commits intomainfrom
refactor-librarianops-generate

Conversation

@miguelvelezsa
Copy link
Contributor

@miguelvelezsa miguelvelezsa commented Mar 19, 2026

Extracting github functions from generate and put them in its own file since upgrade command also needs them.

Also created function uploadToGithub, this functions make easier all the steps needed to upload changes to github. This function will be used in upgrade and generate, the changes for it will be in follow up PR.

For #3911.

Note: coverage complaint is related to generate tests, not to the new file.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request is a good refactoring that moves GitHub-related utility functions into their own file, internal/librarianops/github_utils.go, improving separation of concerns. The addition of tests for these new utilities is also a positive change. However, I've identified a few issues. There's a critical issue with the new branch name generation logic that could cause failures if the tool is run multiple times a day. I've also noted some areas for improvement regarding redundant code, a minor style guide violation, and a bug in the new tests.

miguelvelezsa and others added 3 commits March 19, 2026 16:01
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: miguel <miguelvelezsa@gmail.com>
@codecov
Copy link

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.15%. Comparing base (a29479b) to head (8abc5e1).

Files with missing lines Patch % Lines
internal/librarianops/generate.go 10.00% 7 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4689      +/-   ##
==========================================
+ Coverage   79.10%   79.15%   +0.04%     
==========================================
  Files         111      112       +1     
  Lines        9420     9436      +16     
==========================================
+ Hits         7452     7469      +17     
  Misses       1453     1453              
+ Partials      515      514       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@miguelvelezsa miguelvelezsa marked this pull request as ready for review March 19, 2026 23:32
@miguelvelezsa miguelvelezsa requested a review from a team as a code owner March 19, 2026 23:32
@julieqiu
Copy link
Member

hey @miguelvelezsa, thanks for the PR. I’ve taken a look and would prefer to keep these functions in generate.go for the time being. As we previously discussed, I’d also like to avoid introducing mocks at this stage to keep the testing structure lean. Let's hold off on this refactor for now.

@@ -0,0 +1,76 @@
// Copyright 2026 Google LLC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants