From 183f5ecc131d7e468cdbc6ec39ccb185811790f4 Mon Sep 17 00:00:00 2001 From: Vincent Primault Date: Fri, 6 Sep 2024 15:09:21 +0200 Subject: [PATCH] fix: handle 'is:pr' syntax (#108) Closes #107 --- packages/github/src/client.ts | 27 ++-------------- packages/github/src/search.ts | 42 +++++++++++++++++++++++-- packages/github/tests/search.test.ts | 46 ++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 26 deletions(-) diff --git a/packages/github/src/client.ts b/packages/github/src/client.ts index e8661b0..41f0d81 100644 --- a/packages/github/src/client.ts +++ b/packages/github/src/client.ts @@ -13,7 +13,7 @@ import { type Review, Discussion, } from "@repo/model"; -import { SearchQuery } from "./search.js"; +import { prepareQuery } from "./search.js"; const MAX_PULLS_TO_FETCH = 50; @@ -135,32 +135,11 @@ export class DefaultGitHubClient implements GitHubClient { connection: Connection, search: string, ): Promise { - // Enforce searching for PRs, and filter by org as required by the connection. - const q = new SearchQuery(search); - q.set("type", "pr"); - if (connection.orgs.length > 0) { - q.setAll("org", connection.orgs); - } - if (!q.has("archived")) { - // Unless the query explicitely allows PRs from archived repositories, exclude - // them by default as we cannot act on them anymore. - q.set("archived", "false"); - } - if (q.has("org") && q.has("repo")) { - // GitHub API does not seem to support having both terms with an "org" - // and "repo" qualifier in a given query. In this situation, the term - // with the "repo" qualifier is apparently ignored. We remediate to this - // situation by keeping droping terms with the "org" qualifier, and - // keeping the more precise "repo" qualifier. - // - // Note: We ignore the situation where the targeted repo(s) are not within - // the originally targeted org(s). - q.delete("org"); - } + const q = prepareQuery(search, connection); const octokit = this.getOctokit(connection); const response = await octokit.rest.search.issuesAndPullRequests({ - q: q.toString(), + q, sort: "updated", per_page: MAX_PULLS_TO_FETCH, }); diff --git a/packages/github/src/search.ts b/packages/github/src/search.ts index a39905b..546582d 100644 --- a/packages/github/src/search.ts +++ b/packages/github/src/search.ts @@ -1,3 +1,5 @@ +import { Connection } from "@repo/model"; + /** * Split a search string into multiple GitHub search queries. * @@ -26,6 +28,39 @@ export function splitQueries(search: string): string[] { return queries.map((s) => s.trim()).filter((s) => s.length > 0); } +export function prepareQuery(search: string, connection: Connection): string { + const q = new SearchQuery(search); + // Enforce searching for PRs. There are two syntaxes to search for PRs: "type:pr" + // and "is:pr". We make sure there are no duplicate filters, which would return + // no results. + q.set("type", "pr"); + q.delete("is", "pr"); + + if (!q.has("archived")) { + // Unless the query explicitely allows PRs from archived repositories, exclude + // them by default as we cannot act on them anymore. + q.set("archived", "false"); + } + + // Filter by org as required by the connection. + if (connection.orgs.length > 0) { + q.setAll("org", connection.orgs); + } + if (q.has("org") && q.has("repo")) { + // GitHub API does not seem to support having both terms with an "org" + // and "repo" qualifier in a given query. In this situation, the term + // with the "repo" qualifier is apparently ignored. We remediate to this + // situation by keeping droping terms with the "org" qualifier, and + // keeping the more precise "repo" qualifier. + // + // Note: We ignore the situation where the targeted repo(s) are not within + // the originally targeted org(s). + q.delete("org"); + } + + return q.toString(); +} + /** * Represents a GitHub search query for issues and pull requests. * @@ -66,8 +101,11 @@ export class SearchQuery { ); } - delete(qualifier: string): undefined { - this.terms = this.terms.filter((t) => t.qualifier !== qualifier); + delete(qualifier: string, value?: string): undefined { + const matches = (t: SearchTerm) => + t.qualifier === qualifier && + (value === undefined || (t.values.length === 1 && t.values[0] === value)); + this.terms = this.terms.filter((t) => !matches(t)); } toString(): string { diff --git a/packages/github/tests/search.test.ts b/packages/github/tests/search.test.ts index 3ecf83c..db6515b 100644 --- a/packages/github/tests/search.test.ts +++ b/packages/github/tests/search.test.ts @@ -4,7 +4,9 @@ import { SearchOp, SearchTerm, splitQueries, + prepareQuery, } from "../src/search.js"; +import { mockConnection } from "@repo/testing"; const expectTerms = (str: string, terms: SearchTerm[]) => { const q = new SearchQuery(str); @@ -99,6 +101,16 @@ test("delete term(s) with qualifier", () => { expect(q.toString()).toEqual("stars:>10"); }); +test("delete term(s) with qualifier and value", () => { + const q = new SearchQuery("org:foo org:bar user:pvcnt"); + + q.delete("org", "foobar"); + expect(q.toString()).toEqual("org:foo org:bar user:pvcnt"); + + q.delete("org", "foo"); + expect(q.toString()).toEqual("org:bar user:pvcnt"); +}); + test("set term(s) with qualifier", () => { const q = new SearchQuery("user:pvcnt org:foo org:bar"); @@ -119,3 +131,37 @@ test("split a search string into queries", () => { expect(splitQueries('"a;b";c')).toEqual(["a;b", "c"]); expect(splitQueries("a; ;c")).toEqual(["a", "c"]); }); + +test("should search for non-archived pulls only", () => { + expect(prepareQuery("is:open", mockConnection())).toEqual( + "is:open type:pr archived:false", + ); + + expect(prepareQuery("is:open type:pr", mockConnection())).toEqual( + "is:open type:pr archived:false", + ); + + expect(prepareQuery("is:open is:pr", mockConnection())).toEqual( + "is:open type:pr archived:false", + ); + + expect(prepareQuery("archived:true", mockConnection())).toEqual( + "archived:true type:pr", + ); +}); + +test("should filter by orgs", () => { + const connection = mockConnection({ orgs: ["apache", "kubernetes"] }); + + expect(prepareQuery("is:open", connection)).toEqual( + "is:open type:pr archived:false org:apache org:kubernetes", + ); + + expect(prepareQuery("is:open org:pvcnt", connection)).toEqual( + "is:open type:pr archived:false org:apache org:kubernetes", + ); + + expect(prepareQuery("is:open repo:apache/solr", connection)).toEqual( + "is:open repo:apache/solr type:pr archived:false", + ); +});