Skip to content
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

Refactor API methods #11831

Merged
merged 7 commits into from Aug 9, 2021
Merged

Refactor API methods #11831

merged 7 commits into from Aug 9, 2021

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Aug 6, 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?

This PR refactors all calls to https://formulae.brew.sh/api/ into a new Homebrew::API module. I think this makes it easier to understand what's happening and also allows for some shared logic between them (i.e. fetching and parsing a JSON file). This is mainly motivated by the (in my opinion) poorly-names BottleAPI module. Now, some of the functionality from that module has been moved to Homebrew::API::Bottle, and some to Homebrew::API::Versions.

Now, these are the modules and functions available after using require "api"

  • Homebrew::API: Helper module but can be used for any API call
    • ::fetch(endpoint, json: false): Takes an endpoint (e.g. formula/wget.json) and returns the API response (parsing to JSON if desired)
  • Homebrew::API::Analytics: For /api/analytics or /api/analytics-linux
    • ::fetch(category, days): Makes a request to /api/analytics/{category}/{days}d.json or /api/analytics-linux/{category}/{days}d.json
  • Homebrew::API::Bottle: For /api/bottle or /api/bottle-linux
    • ::fetch(name): Makes a request to /api/bottle/{name}.json or /api/bottle-linux/{name}.json
    • Other methods that already were present in the BottleAPI module including available? and fetch_bottles (among others)
  • Homebrew::API::Cask: For /api/cask (there's no way to call /api/cask.json at the moment because this is not used anywhere)
    • ::fetch(token): Makes a request to /api/cask/{token}.json
  • Homebrew::API::Formula: For /api/formula or /api/formula-linux (there's no way to call /api/formula.json at the moment because this is not used anywhere)
    • ::fetch(name): Makes a request to /api/formula/{name}.json or /api/formula-linux/{name}.json
  • Homebrew::API::Versions: For /api/versions-{formulae,linux,casks}
    • ::formula: Makes a request to /api/versions-formulae
    • ::linux: Makes a request to /api/versions-linux
    • ::casks: Makes a request to /api/versions-casks
    • ::latest_formula_version(name): Returns the PkgVersion of name using /api/versions-formulae or /api/versions-linux
    • ::latest_cask_version(token): Returns the Version of token using /api/versions-casks

@BrewTestBot
Copy link
Member

Review period will end on 2021-08-09 at 08:31:56 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 6, 2021
@Rylan12
Copy link
Member Author

Rylan12 commented Aug 6, 2021

After this PR, I'll open one that adds HOMEBREW_JSON_CORE functionality to homebrew/cask which will use a new API module that I'll add here (and has already been added to formulae.brew.sh): Homebrew::API::CaskSource

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Aug 9, 2021
@BrewTestBot
Copy link
Member

Review period ended.

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.

Love this cleanup. Some improved test coverage would be nice but both that and my comment are non-blocking.

Library/Homebrew/api.rb Outdated Show resolved Hide resolved
Library/Homebrew/api.rb Outdated Show resolved Hide resolved
@Rylan12
Copy link
Member Author

Rylan12 commented Aug 9, 2021

Some improved test coverage would be nice

I wrote some tests for Homebrew::API::fetch by stubbing (or mocking—can't remember which one's which) Utils::Curl::curl_output so it doesn't actually make a network call. I also wrote some tests for the other methods in Homebrew::API::Bottle. Other then that, though, I didn't write tests for the API modules because most of the methods are simply calling Homebrew::API::fetch with different options. It felt like a lot of extra work and tests for limited extended coverage (i.e. one line that just calls a method that we're already testing anyway). I can add more if you think it's important, though

@MikeMcQuaid
Copy link
Member

I wrote some tests for Homebrew::API::fetch by stubbing (or mocking—can't remember which one's which) Utils::Curl::curl_output so it doesn't actually make a network call. I also wrote some tests for the other methods in Homebrew::API::Bottle. Other then that, though, I didn't write tests for the API modules because most of the methods are simply calling Homebrew::API::fetch with different options. It felt like a lot of extra work and tests for limited extended coverage (i.e. one line that just calls a method that we're already testing anyway). I can add more if you think it's important, though

Fine with me 👍🏻

@Rylan12 Rylan12 enabled auto-merge August 9, 2021 15:02
@Rylan12 Rylan12 merged commit 90bbe8b into Homebrew:master Aug 9, 2021
@Rylan12 Rylan12 deleted the api-cleanup branch August 9, 2021 16:32
@carlocab
Copy link
Member

carlocab commented Aug 9, 2021

Error: undefined local variable or method `generic_formula_api_path' for Homebrew::API::Formula:Module

https://github.com/Homebrew/homebrew-core/pull/83028/checks?check_run_id=3283057919#step:7:37

🙈

@Rylan12 Rylan12 mentioned this pull request Aug 9, 2021
7 tasks
@Rylan12
Copy link
Member Author

Rylan12 commented Aug 9, 2021

Oops, thanks! Fix in #11839

@Rylan12
Copy link
Member Author

Rylan12 commented Aug 9, 2021

Should be fixed now @carlocab @dtrodrigues

@github-actions github-actions bot added the outdated PR was locked due to age label Sep 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants