Skip to content

bump: bump synced formula together #19213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bevanjkay
Copy link
Member

@bevanjkay bevanjkay commented Feb 3, 2025

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This PR adds functionality to brew bump that allows Formula with synced versions to be bumped together.
If you pass the flag --bump-synced to brew bump it will automatically include any Formula that are marked as synced versions.

The diff is quite significant in some places, because existing code had to be moved into loops to iterate over the creation of commits and file changes. There isn't great test coverage for these commands as it stands, but some could be added here.

I will keep the branch checked out over the coming days to discover any issues.


Here is an example of a synced version PR opened from this branch:
Homebrew/homebrew-core#219026

@bevanjkay bevanjkay force-pushed the bump-synced-versions branch 4 times, most recently from 2a31737 to 140f11b Compare February 3, 2025 18:22
Copy link
Member

@nandahkrishna nandahkrishna left a comment

Choose a reason for hiding this comment

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

Thanks @bevanjkay, I think the overall logic looks good here – it's just that the diff is a bit scary because everything's been moved into a loop over the list of formulae to bump together (and pr_info contains multiple commits, and only after all these commits are created should the audit checks be run).

Once you get the time to add tests I'd be happy to review this in more detail and even test it out.

@bevanjkay
Copy link
Member Author

I think that ideally I will extract the loop logic into a separate function to make it a bit clearer.
This one has been difficult to test because it's not often that an opportunity arises to bump synced formula - but when I have some time I will try to replicate this in a test tap.

I have ran some singular bumps with this branch checked out, and there is currently an issue where the PR title is incorrect.

Looking to get back to this PR in the next week or two.

Copy link

github-actions bot commented Mar 8, 2025

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Mar 8, 2025
@github-actions github-actions bot closed this Mar 15, 2025
@bevanjkay bevanjkay changed the title [WIP] bump: bump synced formula together bump: bump synced formula together Apr 9, 2025
@bevanjkay bevanjkay reopened this Apr 9, 2025
@bevanjkay bevanjkay force-pushed the bump-synced-versions branch from 140f11b to 4b622b8 Compare April 9, 2025 12:57
@bevanjkay bevanjkay removed the stale No recent activity label Apr 9, 2025
@bevanjkay bevanjkay force-pushed the bump-synced-versions branch from 4b622b8 to afc681e Compare April 9, 2025 12:58
@bevanjkay bevanjkay marked this pull request as ready for review April 9, 2025 13:00
@bevanjkay bevanjkay force-pushed the bump-synced-versions branch 5 times, most recently from 643f008 to a0cd9aa Compare April 9, 2025 13:58
@bevanjkay bevanjkay requested a review from Copilot April 9, 2025 14:10
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/bump.rbi: Language not supported
  • Library/Homebrew/sorbet/rbi/dsl/homebrew/dev_cmd/bump_formula_pr.rbi: Language not supported
Comments suppressed due to low confidence (2)

Library/Homebrew/dev-cmd/bump-formula-pr.rb:410

  • The code uses 'formula' whereas the variable was renamed to 'named_formula' earlier; update this reference for consistency.
sourcefile_path:    formula.path,

Library/Homebrew/dev-cmd/bump-formula-pr.rb:375

  • Replace 'formula' with 'named_formula' in the run_audit call to maintain consistency with the earlier renaming.
run_audit(formula, alias_rename, old_contents, skip_synced_versions: args.bump_synced.present?)

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good! Codepilot feedback actually makes sense but otherwise good to ship.

@bevanjkay bevanjkay force-pushed the bump-synced-versions branch 3 times, most recently from e2d2df9 to 6fb7c81 Compare April 16, 2025 12:43
@bevanjkay
Copy link
Member Author

I'm still doing some further testing here to make sure I'm confident it is working as expected before looking to merge.

@bevanjkay bevanjkay force-pushed the bump-synced-versions branch from 6fb7c81 to 7ccdc34 Compare April 16, 2025 12:49
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.

3 participants