From e93ec12b32f3449f4669cf53469a4d5308b10cc3 Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Sun, 13 Aug 2017 13:10:38 -0400 Subject: [PATCH 01/12] Remove duplicate url generation logic in Github module --- Library/Homebrew/cask/lib/hbc/cli/search.rb | 5 +- Library/Homebrew/cmd/search.rb | 5 +- Library/Homebrew/test/utils/github_spec.rb | 22 +++++++- Library/Homebrew/utils/github.rb | 57 ++++++++++++--------- 4 files changed, 59 insertions(+), 30 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/cli/search.rb b/Library/Homebrew/cask/lib/hbc/cli/search.rb index 643d18d554..0cc4cc56c8 100644 --- a/Library/Homebrew/cask/lib/hbc/cli/search.rb +++ b/Library/Homebrew/cask/lib/hbc/cli/search.rb @@ -15,8 +15,9 @@ module Hbc end def self.search_remote(query) - matches = GitHub.search_code("user:caskroom", "path:Casks", "filename:#{query}", "extension:rb") - [*matches].map do |match| + matches = GitHub.search_code(user: "caskroom", path: "Casks", + filename: query, extension: "rb") + Array(matches).map do |match| tap = Tap.fetch(match["repository"]["full_name"]) next if tap.installed? "#{tap.name}/#{File.basename(match["path"], ".rb")}" diff --git a/Library/Homebrew/cmd/search.rb b/Library/Homebrew/cmd/search.rb index 5b83d80e22..4607f6d1a0 100644 --- a/Library/Homebrew/cmd/search.rb +++ b/Library/Homebrew/cmd/search.rb @@ -102,8 +102,9 @@ module Homebrew def search_taps(query) valid_dirnames = ["Formula", "HomebrewFormula", "Casks", "."].freeze - matches = GitHub.search_code("user:Homebrew", "user:caskroom", "filename:#{query}", "extension:rb") - [*matches].map do |match| + matches = GitHub.search_code(user: ["Homebrew", "caskroom"], filename: query, extension: "rb") + + Array(matches).map do |match| dirname, filename = File.split(match["path"]) next unless valid_dirnames.include?(dirname) tap = Tap.fetch(match["repository"]["full_name"]) diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index 9b539262fb..cf5f6c8080 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -2,12 +2,30 @@ require "utils/github" describe GitHub do describe "::search_code", :needs_network do - it "searches code" do - results = subject.search_code("repo:Homebrew/brew", "path:/", "filename:readme", "language:markdown") + it "queries GitHub code with the passed paramaters" do + results = subject.search_code(repo: "Homebrew/brew", path: "/", + filename: "readme", language: "markdown") expect(results.count).to eq(1) expect(results.first["name"]).to eq("README.md") expect(results.first["path"]).to eq("README.md") end end + + describe "::query_string" do + it "builds a query with the given hash parameters formatted as key:value" do + query = subject.query_string(user: "Homebrew", repo: "Brew") + expect(query).to eq("q=user%3aHomebrew+repo%3aTest&per_page=100") + end + + it "adds a variable number of top-level string parameters to the query when provided" do + query = subject.query_string("value1", "value2", user: "Homebrew") + expect(query).to eq("q=value1+value2+user%3aHomebrew&per_page=100") + end + + it "turns array values into multiple key:value parameters" do + query = subject.query_string(user: ["Homebrew", "caskroom"]) + expect(query).to eq("q=user%3aHomebrew+user%3acaskroom&per_page=100") + end + end end diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 07eea43849..6f56e505c5 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -228,34 +228,15 @@ module GitHub end def issues_matching(query, qualifiers = {}) - uri = URI.parse("#{API_URL}/search/issues") - uri.query = build_query_string(query, qualifiers) - open(uri) { |json| json["items"] } + search("issues", query, **qualifiers) end def repository(user, repo) - open(URI.parse("#{API_URL}/repos/#{user}/#{repo}")) + open(path_to("repos", user, repo)) end - def search_code(*params) - uri = URI.parse("#{API_URL}/search/code") - uri.query = "q=#{uri_escape(params.join(" "))}" - open(uri) { |json| json["items"] } - end - - def build_query_string(query, qualifiers) - s = "q=#{uri_escape(query)}+" - s << build_search_qualifier_string(qualifiers) - s << "&per_page=100" - end - - def build_search_qualifier_string(qualifiers) - { - repo: "Homebrew/homebrew-core", - in: "title", - }.update(qualifiers).map do |qualifier, value| - "#{qualifier}:#{value}" - end.join("+") + def search_code(**params) + search("code", **params) end def uri_escape(query) @@ -292,7 +273,35 @@ module GitHub end def private_repo?(full_name) - uri = URI.parse("#{API_URL}/repos/#{full_name}") + uri = path_to "repos", full_name open(uri) { |json| json["private"] } end + + def query_string(*main_params, **qualifiers) + params_list = main_params + + qualifiers.each do |key, value| + if value.is_a? Array + value.each { |v| params_list << format_parameter(key, v) } + else + params_list << format_parameter(key, v) + end + end + + "q=#{uri_escape(params_list.join(" "))}&per_page=100" + end + + def format_paramater(key, value) + "#{key}:#{value}" + end + + def path_to(*subroutes) + URI.parse(File.join(API_URL, *subroutes)) + end + + def search(entity, *queries, **qualifiers) + uri = path_to "search", entity + uri.query = query_string(*queries, **qualifiers) + open(uri) { |json| json["items"] } + end end From acf46fce5f08712084dc9454fee66637796055c6 Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Sun, 13 Aug 2017 15:01:37 -0400 Subject: [PATCH 02/12] Rename path_to -> url_to since it returns a url --- Library/Homebrew/utils/github.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 6f56e505c5..c10a8ed0da 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -232,7 +232,7 @@ module GitHub end def repository(user, repo) - open(path_to("repos", user, repo)) + open(url_to("repos", user, repo)) end def search_code(**params) @@ -273,7 +273,7 @@ module GitHub end def private_repo?(full_name) - uri = path_to "repos", full_name + uri = url_to "repos", full_name open(uri) { |json| json["private"] } end @@ -295,12 +295,12 @@ module GitHub "#{key}:#{value}" end - def path_to(*subroutes) + def url_to(*subroutes) URI.parse(File.join(API_URL, *subroutes)) end def search(entity, *queries, **qualifiers) - uri = path_to "search", entity + uri = url_to "search", entity uri.query = query_string(*queries, **qualifiers) open(uri) { |json| json["items"] } end From 91b139aca293bea1aa926c5301754e8603c9275e Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Sun, 13 Aug 2017 16:15:26 -0400 Subject: [PATCH 03/12] Fix syntax error --- Library/Homebrew/utils/github.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index c10a8ed0da..59c7d7ac85 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -284,7 +284,7 @@ module GitHub if value.is_a? Array value.each { |v| params_list << format_parameter(key, v) } else - params_list << format_parameter(key, v) + params_list << format_parameter(key, value) end end From d052f503f90ad08f6f889891e3fb72e97988a80e Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Sun, 13 Aug 2017 16:41:18 -0400 Subject: [PATCH 04/12] fix typo --- Library/Homebrew/utils/github.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 59c7d7ac85..05ef7e3c86 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -291,7 +291,7 @@ module GitHub "q=#{uri_escape(params_list.join(" "))}&per_page=100" end - def format_paramater(key, value) + def format_parameter(key, value) "#{key}:#{value}" end From 24da1ecd3de86177f669072c542c9a01e6d6649d Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Sun, 13 Aug 2017 17:16:45 -0400 Subject: [PATCH 05/12] Fix url encoded in expected values --- Library/Homebrew/test/utils/github_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index cf5f6c8080..79a26083a3 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -15,17 +15,17 @@ describe GitHub do describe "::query_string" do it "builds a query with the given hash parameters formatted as key:value" do query = subject.query_string(user: "Homebrew", repo: "Brew") - expect(query).to eq("q=user%3aHomebrew+repo%3aTest&per_page=100") + expect(query).to eq("q=user%3AHomebrew+repo%3ABrew&per_page=100") end it "adds a variable number of top-level string parameters to the query when provided" do query = subject.query_string("value1", "value2", user: "Homebrew") - expect(query).to eq("q=value1+value2+user%3aHomebrew&per_page=100") + expect(query).to eq("q=value1+value2+user%3AHomebrew&per_page=100") end it "turns array values into multiple key:value parameters" do query = subject.query_string(user: ["Homebrew", "caskroom"]) - expect(query).to eq("q=user%3aHomebrew+user%3acaskroom&per_page=100") + expect(query).to eq("q=user%3AHomebrew+user%3Acaskroom&per_page=100") end end end From 603bdd01a81e62d6b97a5da26d75d409e839a8fa Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Mon, 14 Aug 2017 11:08:56 -0400 Subject: [PATCH 06/12] Implement PR feedback --- Library/Homebrew/cask/lib/hbc/cli/search.rb | 2 +- Library/Homebrew/cmd/search.rb | 2 +- Library/Homebrew/test/utils/github_spec.rb | 4 ++-- Library/Homebrew/utils/github.rb | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/cli/search.rb b/Library/Homebrew/cask/lib/hbc/cli/search.rb index 0cc4cc56c8..e89dced925 100644 --- a/Library/Homebrew/cask/lib/hbc/cli/search.rb +++ b/Library/Homebrew/cask/lib/hbc/cli/search.rb @@ -17,7 +17,7 @@ module Hbc def self.search_remote(query) matches = GitHub.search_code(user: "caskroom", path: "Casks", filename: query, extension: "rb") - Array(matches).map do |match| + matches.map do |match| tap = Tap.fetch(match["repository"]["full_name"]) next if tap.installed? "#{tap.name}/#{File.basename(match["path"], ".rb")}" diff --git a/Library/Homebrew/cmd/search.rb b/Library/Homebrew/cmd/search.rb index 4607f6d1a0..a133d04999 100644 --- a/Library/Homebrew/cmd/search.rb +++ b/Library/Homebrew/cmd/search.rb @@ -104,7 +104,7 @@ module Homebrew valid_dirnames = ["Formula", "HomebrewFormula", "Casks", "."].freeze matches = GitHub.search_code(user: ["Homebrew", "caskroom"], filename: query, extension: "rb") - Array(matches).map do |match| + matches.map do |match| dirname, filename = File.split(match["path"]) next unless valid_dirnames.include?(dirname) tap = Tap.fetch(match["repository"]["full_name"]) diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index 79a26083a3..5c315aec4a 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -14,8 +14,8 @@ describe GitHub do describe "::query_string" do it "builds a query with the given hash parameters formatted as key:value" do - query = subject.query_string(user: "Homebrew", repo: "Brew") - expect(query).to eq("q=user%3AHomebrew+repo%3ABrew&per_page=100") + query = subject.query_string(user: "Homebrew", repo: "brew") + expect(query).to eq("q=user%3AHomebrew+repo%3Abrew&per_page=100") end it "adds a variable number of top-level string parameters to the query when provided" do diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 05ef7e3c86..960b563d91 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -296,12 +296,12 @@ module GitHub end def url_to(*subroutes) - URI.parse(File.join(API_URL, *subroutes)) + URI.parse([API_URL, *subroutes].join("/")) end def search(entity, *queries, **qualifiers) uri = url_to "search", entity uri.query = query_string(*queries, **qualifiers) - open(uri) { |json| json["items"] } + open(uri) { |json| Array(json["items"]) } end end From 68cdb550f7b90318a136e4dd484249ab678f5fbc Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Mon, 14 Aug 2017 11:41:29 -0400 Subject: [PATCH 07/12] Spec for issues search --- Library/Homebrew/test/utils/github_spec.rb | 8 ++++++++ Library/Homebrew/utils/github.rb | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index 5c315aec4a..80f0ef7861 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -28,4 +28,12 @@ describe GitHub do expect(query).to eq("q=user%3AHomebrew+user%3Acaskroom&per_page=100") end end + + describe "::issues_matching", :needs_network do + it "queries GitHub issues with the passed parameters" do + results = subject.issues_matching("brew search", repo: "Homebrew/brew", author: "avetamine", is: "closed") + expect(results.count).to eq(1) + expect(results.first["title"]).to eq("brew search : 422 Unprocessable Entity") + end + end end diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 960b563d91..8096c7b9c4 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -266,7 +266,7 @@ module GitHub puts "Closed pull requests:" prs = open_or_closed_prs else - return + return [] end prs.each { |i| puts "#{i["title"]} (#{i["html_url"]})" } From 5f8d212ccccb8172985da9f71945fea6a8ea1b1a Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Mon, 14 Aug 2017 12:51:32 -0400 Subject: [PATCH 08/12] Unify vocabulary in github module, remove unnecessary logic Fix duplication bug --- Library/Homebrew/test/utils/github_spec.rb | 4 +- Library/Homebrew/utils/github.rb | 43 ++++++---------------- 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index 80f0ef7861..0b19c8c9e9 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -29,9 +29,9 @@ describe GitHub do end end - describe "::issues_matching", :needs_network do + describe "::search_issues", :needs_network do it "queries GitHub issues with the passed parameters" do - results = subject.issues_matching("brew search", repo: "Homebrew/brew", author: "avetamine", is: "closed") + results = subject.search_issues("brew search", repo: "Homebrew/brew", author: "avetamine", is: "closed") expect(results.count).to eq(1) expect(results.first["title"]).to eq("brew search : 422 Unprocessable Entity") end diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index 8096c7b9c4..cff292598a 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -227,7 +227,7 @@ module GitHub end end - def issues_matching(query, qualifiers = {}) + def search_issues(query, **qualifiers) search("issues", query, **qualifiers) end @@ -235,38 +235,27 @@ module GitHub open(url_to("repos", user, repo)) end - def search_code(**params) - search("code", **params) - end - - def uri_escape(query) - if URI.respond_to?(:encode_www_form_component) - URI.encode_www_form_component(query) - else - require "erb" - ERB::Util.url_encode(query) - end + def search_code(**qualifiers) + search("code", **qualifiers) end def issues_for_formula(name, options = {}) tap = options[:tap] || CoreTap.instance - issues_matching(name, state: "open", repo: "#{tap.user}/homebrew-#{tap.repo}") + search_issues(name, state: "open", repo: "#{tap.user}/homebrew-#{tap.repo}") end def print_pull_requests_matching(query) return [] if ENV["HOMEBREW_NO_GITHUB_API"] - open_or_closed_prs = issues_matching(query, type: "pr") + open_or_closed_prs = search_issues(query, type: "pr") open_prs = open_or_closed_prs.select { |i| i["state"] == "open" } - if !open_prs.empty? + prs = if !open_prs.empty? puts "Open pull requests:" - prs = open_prs - elsif !open_or_closed_prs.empty? - puts "Closed pull requests:" - prs = open_or_closed_prs + open_prs else - return [] + puts "Closed pull requests:" unless open_or_closed_prs.empty? + open_or_closed_prs end prs.each { |i| puts "#{i["title"]} (#{i["html_url"]})" } @@ -280,19 +269,11 @@ module GitHub def query_string(*main_params, **qualifiers) params_list = main_params - qualifiers.each do |key, value| - if value.is_a? Array - value.each { |v| params_list << format_parameter(key, v) } - else - params_list << format_parameter(key, value) - end + params_list += qualifiers.flat_map do |key, value| + Array(value).map { |v| "#{key}:#{v}" } end - "q=#{uri_escape(params_list.join(" "))}&per_page=100" - end - - def format_parameter(key, value) - "#{key}:#{value}" + "q=#{URI.encode_www_form_component(params_list.join(" "))}&per_page=100" end def url_to(*subroutes) From 7a93638105474c8e17a115dd2c16ae52f8f6806f Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Thu, 17 Aug 2017 11:40:32 -0400 Subject: [PATCH 09/12] Make test more resilient --- Library/Homebrew/test/utils/github_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index 0b19c8c9e9..7bfb4a9308 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -32,8 +32,8 @@ describe GitHub do describe "::search_issues", :needs_network do it "queries GitHub issues with the passed parameters" do results = subject.search_issues("brew search", repo: "Homebrew/brew", author: "avetamine", is: "closed") - expect(results.count).to eq(1) - expect(results.first["title"]).to eq("brew search : 422 Unprocessable Entity") + expect(results.count).to be > 1 + expect(results.last["title"]).to eq("brew search : 422 Unprocessable Entity") end end end From e096836b7b46e605fb4d1c10c632157dd6b11186 Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Thu, 17 Aug 2017 11:57:58 -0400 Subject: [PATCH 10/12] Improve rspec readability --- Library/Homebrew/test/utils/github_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index 7bfb4a9308..151b6430d1 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -32,7 +32,7 @@ describe GitHub do describe "::search_issues", :needs_network do it "queries GitHub issues with the passed parameters" do results = subject.search_issues("brew search", repo: "Homebrew/brew", author: "avetamine", is: "closed") - expect(results.count).to be > 1 + expect(results.count).not_to be_empty expect(results.last["title"]).to eq("brew search : 422 Unprocessable Entity") end end From 564a06dfbb711a995558c64c55b353d1fdfcb473 Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Thu, 17 Aug 2017 13:44:18 -0400 Subject: [PATCH 11/12] Fix typo in spec --- Library/Homebrew/test/utils/github_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Library/Homebrew/test/utils/github_spec.rb b/Library/Homebrew/test/utils/github_spec.rb index 151b6430d1..9322898eee 100644 --- a/Library/Homebrew/test/utils/github_spec.rb +++ b/Library/Homebrew/test/utils/github_spec.rb @@ -32,7 +32,7 @@ describe GitHub do describe "::search_issues", :needs_network do it "queries GitHub issues with the passed parameters" do results = subject.search_issues("brew search", repo: "Homebrew/brew", author: "avetamine", is: "closed") - expect(results.count).not_to be_empty + expect(results).not_to be_empty expect(results.last["title"]).to eq("brew search : 422 Unprocessable Entity") end end From ca05c7f010ed952b316fe541f94a8fc2a83446ec Mon Sep 17 00:00:00 2001 From: Ben Muschol Date: Thu, 17 Aug 2017 14:38:50 -0400 Subject: [PATCH 12/12] Change var name --- Library/Homebrew/utils/github.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Library/Homebrew/utils/github.rb b/Library/Homebrew/utils/github.rb index cff292598a..578d967a49 100644 --- a/Library/Homebrew/utils/github.rb +++ b/Library/Homebrew/utils/github.rb @@ -267,13 +267,13 @@ module GitHub end def query_string(*main_params, **qualifiers) - params_list = main_params + params = main_params - params_list += qualifiers.flat_map do |key, value| + params += qualifiers.flat_map do |key, value| Array(value).map { |v| "#{key}:#{v}" } end - "q=#{URI.encode_www_form_component(params_list.join(" "))}&per_page=100" + "q=#{URI.encode_www_form_component(params.join(" "))}&per_page=100" end def url_to(*subroutes)