Skip to content

bundle/brew_services: avoid parsing output of brew services list#21830

Merged
carlocab merged 2 commits intomainfrom
services-list-json
Mar 25, 2026
Merged

bundle/brew_services: avoid parsing output of brew services list#21830
carlocab merged 2 commits intomainfrom
services-list-json

Conversation

@carlocab
Copy link
Copy Markdown
Member

  • 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 (excluding integration tests) for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

  • AI was used to generate or assist with generating this PR. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes.

brew services can output JSON, so let's use that instead since it
should be more reliable.

Copilot AI review requested due to automatic review settings March 25, 2026 05:55
Copy link
Copy Markdown
Contributor

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.

Pull request overview

Updates brew bundle’s service detection to use brew services list --json instead of parsing the human-readable table output, aiming for more reliable parsing.

Changes:

  • Switch Homebrew::Bundle::Brew::Services.started_services to parse JSON from brew services list --json.
  • Update the corresponding RSpec to stub JSON output instead of the table output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
Library/Homebrew/bundle/brew_services.rb Changes started_services to use JSON output from brew services list --json.
Library/Homebrew/test/bundle/brew_services_spec.rb Updates the test fixture output to JSON to match the new implementation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

`brew services` can output JSON, so let's use that instead since it
should be more reliable.
@carlocab carlocab force-pushed the services-list-json branch from 3ccf253 to ac06ea7 Compare March 25, 2026 06:14
`brew services list --json` was returning early without any output when
there were no services, causing `JSON.parse` to raise `JSON::ParserError`
in callers expecting valid JSON.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
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.

Much nicer, thanks @carlocab!

Now that bundle and services are both in the same repository we could consider dropping the shelling out entirely and just calling the native Homebrew methods?

@carlocab
Copy link
Copy Markdown
Member Author

carlocab commented Mar 25, 2026

Now that bundle and services are both in the same repository we could consider dropping the shelling out entirely and just calling the native Homebrew methods?

Was considering that, but I wasn't yet sure that we didn't have other reasons for shelling out that still applied. I'll look at it in a follow-up.

@carlocab carlocab added this pull request to the merge queue Mar 25, 2026
Merged via the queue into main with commit 8bc5b18 Mar 25, 2026
44 checks passed
@carlocab carlocab deleted the services-list-json branch March 25, 2026 11:03
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