utils/github: set default args to search_code #10363

hyuraku · 2021-01-19T12:59:18Z

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?
Have you successfully run brew man locally and committed any changes?

set default args to def search_code in utils/github

MikeMcQuaid

Nice work so far @hyuraku!

MikeMcQuaid · 2021-01-19T13:49:59Z

Library/Homebrew/utils/github.rb

@@ -309,8 +309,8 @@ def repository(user, repo)
    open_api(url_to("repos", user, repo))
  end

-  def search_code(**qualifiers)
-    matches = search("code", **qualifiers)
+  def search_code(user: "Homebrew", path: ["Formula", "Casks", "."], filename: nil, extension: "rb")


I think these defaults except filename: nil are better kept in the search.rb

I don't think it makes sense to use keyword arguments here since this should support all fields supported by GitHub search. Also I think there may definitely be some places in other repos where repo is used.

I think Homebrew::Search.search_taps can pass only query to GitHub.search_code and it easier to read when I set default keyword args.

While I couldn't seem to find usage of search_code with the repo field (in Homebrew/brew), I agree with @reitermarkus that the method should support all fields that GitHub's search does. We probably shouldn't be removing the ability to use these keys based on just the usage in search_taps.

MikeMcQuaid · 2021-01-19T13:50:14Z

Library/Homebrew/utils/github.rb

-  def search_code(**qualifiers)
-    matches = search("code", **qualifiers)
+  def search_code(user: "Homebrew", path: ["Formula", "Casks", "."], filename: nil, extension: "rb")
+    matches = search("code", **{ user: user, path: path, filename: filename, extension: extension })


Would be nicer to pass these as actual keyword arguments too, perhaps?

MikeMcQuaid · 2021-01-19T13:50:15Z

Library/Homebrew/utils/github.rb

@@ -309,8 +309,8 @@ def repository(user, repo)
    open_api(url_to("repos", user, repo))
  end

-  def search_code(**qualifiers)
-    matches = search("code", **qualifiers)
+  def search_code(user: "Homebrew", path: ["Formula", "Casks", "."], filename: nil, extension: "rb")


I think these defaults except filename: nil are better kept in the search.rb

hyuraku · 2021-01-19T16:33:50Z

Library/Homebrew/test/utils/github_spec.rb

@@ -6,8 +6,7 @@
 describe GitHub do
  describe "::search_code", :needs_network do
    it "queries GitHub code with the passed parameters" do
-      results = described_class.search_code(repo: "Homebrew/brew", path: "/",


remove repo and language because these keywords are not used in search.rb

MikeMcQuaid · 2021-02-02T11:59:50Z

@hyuraku Can you try to fix up the brew tests failures? Thanks!

hyuraku · 2021-02-02T12:09:39Z

@hyuraku Can you try to fix up the brew tests failures? Thanks!

Yes, I'll fix and brew tests --online

MikeMcQuaid · 2021-02-03T08:24:13Z

Library/Homebrew/test/utils/github_spec.rb

-      expect(results.count).to eq(1)
+      expect(results.count).to eq(29)


Hmm, this looks like it's changed behaviour if the results count has increased by so much?

This change retrieves README.md files (present at /README.md) from the 29 public repositories rather than just from Homebrew/brew.

I think the previous test was more in line with real-world usage. It's more likely that we search a specific repo rather than all in the org.

@hyuraku Please update this so the test no longer requires eq(29), thanks!

MikeMcQuaid · 2021-02-08T11:51:41Z

Library/Homebrew/test/utils/github_spec.rb

-      expect(results.first["name"]).to eq("README.md")
-      expect(results.first["path"]).to eq("README.md")
+      expect(results.first["name"]).to eq("1password-cli.rb")
+      expect(results.first["path"]).to eq("Casks/1password-cli.rb")


Please leave this as-is; I'd like to see this PR not require any test changes.

Sorry, I'll reset this test.

No problem!

@hyuraku Any news here?

@MikeMcQuaid
I just updated!

MikeMcQuaid

Looks like test still failing unfortunately 😭

MikeMcQuaid

LGTM, thanks @hyuraku!

MikeMcQuaid reviewed Jan 19, 2021

View reviewed changes

hyuraku force-pushed the utils/github_set_default_args branch from c40305a to 6fcb74d Compare January 19, 2021 15:18

hyuraku commented Jan 19, 2021

View reviewed changes

hyuraku force-pushed the utils/github_set_default_args branch from 352e023 to f54ef29 Compare February 2, 2021 13:10

MikeMcQuaid reviewed Feb 3, 2021

View reviewed changes

hyuraku force-pushed the utils/github_set_default_args branch from f54ef29 to b997e7e Compare February 8, 2021 11:37

MikeMcQuaid reviewed Feb 8, 2021

View reviewed changes

hyuraku force-pushed the utils/github_set_default_args branch from b997e7e to defa4af Compare February 26, 2021 12:10

utils/github: set default args to search_code

178ae75

MikeMcQuaid reviewed Feb 26, 2021

View reviewed changes

hyuraku force-pushed the utils/github_set_default_args branch from defa4af to 178ae75 Compare February 26, 2021 12:43

MikeMcQuaid approved these changes Feb 26, 2021

View reviewed changes

MikeMcQuaid merged commit f698bb1 into Homebrew:master Feb 26, 2021

hyuraku deleted the utils/github_set_default_args branch February 27, 2021 03:40

BrewTestBot added the outdated PR was locked due to age label Mar 29, 2021

Homebrew locked as resolved and limited conversation to collaborators Mar 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

utils/github: set default args to search_code #10363

utils/github: set default args to search_code #10363

hyuraku commented Jan 19, 2021

MikeMcQuaid left a comment

MikeMcQuaid Jan 19, 2021

reitermarkus Jan 19, 2021

hyuraku Jan 22, 2021 •

edited

nandahkrishna Feb 3, 2021 •

edited

MikeMcQuaid Jan 19, 2021

MikeMcQuaid Jan 19, 2021

hyuraku Jan 19, 2021

MikeMcQuaid commented Feb 2, 2021

hyuraku commented Feb 2, 2021

MikeMcQuaid Feb 3, 2021

nandahkrishna Feb 3, 2021

reitermarkus Feb 7, 2021

MikeMcQuaid Feb 8, 2021

MikeMcQuaid Feb 8, 2021

hyuraku Feb 8, 2021

MikeMcQuaid Feb 8, 2021

MikeMcQuaid Feb 26, 2021

hyuraku Feb 26, 2021

MikeMcQuaid left a comment

MikeMcQuaid left a comment

		expect(results.count).to eq(1)
		expect(results.count).to eq(29)

utils/github: set default args to search_code #10363

utils/github: set default args to search_code #10363

Conversation

hyuraku commented Jan 19, 2021

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyuraku Jan 22, 2021 • edited

Choose a reason for hiding this comment

nandahkrishna Feb 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 2, 2021

hyuraku commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

hyuraku Jan 22, 2021 •

edited

nandahkrishna Feb 3, 2021 •

edited