Skip to content

Conversation

zkokaja
Copy link

@zkokaja zkokaja commented Jan 10, 2021

  • 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?
    I get one failure on an unrelated test dev-cmd/tap-new:11 because git outputs a warning regarding the init.defaultBranch setting on new repos, and the test expected nothing in standard error.
  • Have you successfully run brew man locally and committed any changes?

This commit adds two new values to the brew info --cask --json=v2 command to include whether the cask is outdated and the currently installed versions.

I had to modify one test script to match the new JSON output in order to pass.

"url" => url,
"appcast" => appcast,
"version" => version,
"installed" => versions,
Copy link
Member

Choose a reason for hiding this comment

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

This should be a boolean.

Copy link
Author

Choose a reason for hiding this comment

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

I was trying to match the keys with formula.rb which lists installed kegs with the installed key. I see why it makes sense as a boolean though. How about changing it to a boolean, and then adding "installed_versions" => versions to provide additional details?

Copy link
Contributor

@vitorgalvao vitorgalvao Jan 24, 2021

Choose a reason for hiding this comment

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

How about changing it to a boolean, and then adding "installed_versions" => versions to provide additional details?

You can’t have more than one version of a cask installed at once—that would make no sense for most casks. So just the boolean, and no further details necessary.

@zkokaja
Copy link
Author

zkokaja commented Jan 15, 2021

Any suggestions regarding using installed as a boolean, and installed_versions to list out the installed versions?

@zkokaja
Copy link
Author

zkokaja commented Jan 23, 2021

Hi, I wanted to give a better description of this request and why it's needed. I think I should have opened an issue first to discuss before submitting the PR (sorry! first time contributing to open source). @reitermarkus, please let me know if you'd like me to open an issue first, or if I should make further changes. Thanks!

Description: The goal of this change is to add information on cask installed versions when using the JSON API, as well as to make it more similar to the JSON output for formulae. For example, brew info --json=v2 jq includes all this information:

      "installed": [
        {
          "version": "1.6",
          "used_options": [],
          "built_as_bottle": true,
          "poured_from_bottle": true,
          "runtime_dependencies": [
            {
              "full_name": "oniguruma",
              "version": "6.9.6"
            }
          ],
          "installed_as_dependency": false,
          "installed_on_request": true
        }
      ],
      "outdated": false,

Whereas if I run brew info --json=v2 --cask rabbitmq I get no output regarding what version is installed or whether it's outdated.

Justification: Adding these keys to the output makes it easier for downstream users of the API to use the info command to get a complete summary of the brew state without needing to coordinating calls between info and outdated. Also, making the API similar between formulae and casks is advantageous for consistency and consumption.

Use case: I plan to use this in my https://github.com/zkokaja/Brewlet application to notify users when casks are out of date, and show them the versions they have.

Open question: Should installed be a boolean and installed_versions be a list of installed versions? Or should installed be similar to the output for formulae?

Copy link
Contributor

@SeekingMeaning SeekingMeaning left a comment

Choose a reason for hiding this comment

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

LGTM; if necessary, we could make changes in follow-up PRs

@MikeMcQuaid MikeMcQuaid requested a review from a team January 25, 2021 10:03
@MikeMcQuaid MikeMcQuaid merged commit 95c6e92 into Homebrew:master Jan 25, 2021
@MikeMcQuaid
Copy link
Member

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @zkokaja!

@zkokaja
Copy link
Author

zkokaja commented Jan 25, 2021

Awesome, thank you!! 😊

@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Feb 25, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Feb 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
6 participants