Skip to content

Commit

Permalink
Groups repo+prID pairs by provider to perform the more precise search
Browse files Browse the repository at this point in the history
  • Loading branch information
sergeibbb committed Dec 18, 2024
1 parent ec53d49 commit 816809d
Show file tree
Hide file tree
Showing 14 changed files with 365 additions and 126 deletions.
61 changes: 14 additions & 47 deletions src/git/models/__tests__/pullRequest.utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,61 +2,28 @@ import * as assert from 'assert';
import { suite, test } from 'mocha';
import { getPullRequestIdentityValuesFromSearch } from '../pullRequest.utils';

suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => {
suite('Test PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => {
function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
assert.deepStrictEqual(
getPullRequestIdentityValuesFromSearch(query),
{
ownerAndRepo: ownerAndRepo,
prNumber: prNumber,
},
prNumber == null
? undefined
: {
ownerAndRepo: ownerAndRepo,
prNumber: prNumber,
},
`${message} (${JSON.stringify(query)})`,
);
}

test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => {
t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens');
t(
'with suffix',
'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello',
'1',
'eamodio/vscode-gitlens',
);
t(
'with query',
'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello',
'1',
'eamodio/vscode-gitlens',
);

t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens');
t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens');
t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1');

t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1');
t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
});

test('no domain, should parse to ownerAndRepo and prNumber', () => {
t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1');
t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens');
t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
});

test('domain vs. no domain', () => {
t(
'with anchor',
'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16',
'1',
'eamodio/vscode-gitlens',
);
});
test('cannot recognize GitHub or GitLab URLs, sees only numbers', () => {
t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16');
t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '1');

test('has "pull/" fragment', () => {
t('with leading slash', '/pull/16/files#hello', '16');
t('without leading slash', 'pull/16?diff=unified#hello', '16');
t('with numeric repo name', '1/pull/16?diff=unified#hello', '16');
t('with double slash', '1//pull/16?diff=unified#hello', '16');
t('no protocol', '/github.com/sergeibbb/1/pull/16?diff=unified', '1');
t('no domain', '/sergeibbb/1/pull/16#hello', '1');
t('domain vs. no domain', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/2/pull/16', '1');
t('has "pull/" fragment', '/pull/16/files#hello', '16');
});

test('has "/<num>" fragment', () => {
Expand Down
3 changes: 2 additions & 1 deletion src/git/models/pullRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -420,11 +420,12 @@ export async function getOpenedPullRequestRepo(

export function doesPullRequestSatisfyRepositoryURLIdentity(
pr: EnrichablePullRequest | undefined,
{ ownerAndRepo, prNumber }: PullRequestUrlIdentity,
prUrlIdentity: { [key in string]?: PullRequestUrlIdentity },
): boolean {
if (pr == null) {
return false;
}
const { ownerAndRepo, prNumber } = prUrlIdentity[pr.provider.id] ?? {};
const satisfiesPrNumber = prNumber != null && pr.number === parseInt(prNumber, 10);
if (!satisfiesPrNumber) {
return false;
Expand Down
27 changes: 6 additions & 21 deletions src/git/models/pullRequest.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,16 @@ export interface PullRequestUrlIdentity {
provider?: HostingIntegrationId;

ownerAndRepo?: string;
prNumber?: string;
prNumber: string;
}

export function getPullRequestIdentityValuesFromSearch(search: string): PullRequestUrlIdentity {
let ownerAndRepo: string | undefined = undefined;
export function getPullRequestIdentityValuesFromSearch(search: string): PullRequestUrlIdentity | undefined {
const ownerAndRepo: string | undefined = undefined;
let prNumber: string | undefined = undefined;

let match = search.match(/([^/]+\/[^/]+)\/(?:pull|-\/merge_requests)\/(\d+)/); // with org and rep name
let match = search.match(/(?:\/)(\d+)/); // any number starting with "/"
if (match != null) {
ownerAndRepo = match[1];
prNumber = match[2];
}

if (prNumber == null) {
match = search.match(/(?:\/|^)(?:pull|-\/merge_requests)\/(\d+)/); // without repo name
if (match != null) {
prNumber = match[1];
}
}

if (prNumber == null) {
match = search.match(/(?:\/)(\d+)/); // any number starting with "/"
if (match != null) {
prNumber = match[1];
}
prNumber = match[1];
}

if (prNumber == null) {
Expand All @@ -42,5 +27,5 @@ export function getPullRequestIdentityValuesFromSearch(search: string): PullRequ
}
}

return { ownerAndRepo: ownerAndRepo, prNumber: prNumber };
return prNumber == null ? undefined : { ownerAndRepo: ownerAndRepo, prNumber: prNumber };
}
3 changes: 3 additions & 0 deletions src/plus/integrations/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {
PullRequestState,
SearchedPullRequest,
} from '../../git/models/pullRequest';
import type { PullRequestUrlIdentity } from '../../git/models/pullRequest.utils';
import type { RepositoryMetadata } from '../../git/models/repositoryMetadata';
import { showIntegrationDisconnectedTooManyFailedRequestsWarningMessage } from '../../messages';
import { gate } from '../../system/decorators/gate';
Expand Down Expand Up @@ -1361,4 +1362,6 @@ export abstract class HostingIntegration<
repos?: T[],
cancellation?: CancellationToken,
): Promise<PullRequest[] | undefined>;

getPullRequestIdentityValuesFromSearch?(search: string): PullRequestUrlIdentity | undefined;
}
80 changes: 80 additions & 0 deletions src/plus/integrations/providers/__tests__/github.utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import * as assert from 'assert';
import { suite, test } from 'mocha';
import { getGitHubPullRequestIdentityValuesFromSearch } from '../github.utils';

suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => {
function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
assert.deepStrictEqual(
getGitHubPullRequestIdentityValuesFromSearch(query),
prNumber == null
? undefined
: {
ownerAndRepo: ownerAndRepo,
prNumber: prNumber,
},
`${message} (${JSON.stringify(query)})`,
);
}

test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => {
t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens');
t(
'with suffix',
'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello',
'1',
'eamodio/vscode-gitlens',
);
t(
'with query',
'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello',
'1',
'eamodio/vscode-gitlens',
);

t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens');
t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens');
t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1');

t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1');
t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
});

test('no domain, should parse to ownerAndRepo and prNumber', () => {
t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1');
t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens');
t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
});

test('domain vs. no domain', () => {
t(
'with anchor',
'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16',
'1',
'eamodio/vscode-gitlens',
);
});

test('numbers', () => {
t('has "pull/" fragment', '/pull/16/files#hello', '16');
t('has "pull/" fragment with double slash', '1//pull/16?diff=unified#hello', '16');
t('with leading slash', '/16/files#hello', '16');
t('just a number', '16', '16');
t('with a hash', '#16', '16');
});

test('does not match', () => {
t('without leading slash', '16?diff=unified#hello', undefined);
t('with leading hash', '/#16/files#hello', undefined);
t('number is a part of a word', 'hello16', undefined);
t('number is a part of a word', '16hello', undefined);

t('GitLab', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/16', '16');

t('with a number', '1/16?diff=unified#hello', '16');
t('with a number and slash', '/1/16?diff=unified#hello', '1');
t('with a word', 'anything/16?diff=unified#hello', '16');

t('with a wrong character leading to pull', 'sergeibbb/1/-pull/16?diff=unified#hello', '1');
t('with a wrong character leading to pull', 'sergeibbb/1-pull/16?diff=unified#hello', '1');
});
});
107 changes: 107 additions & 0 deletions src/plus/integrations/providers/__tests__/gitlab.utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import * as assert from 'assert';
import { suite, test } from 'mocha';
import { getGitLabPullRequestIdentityValuesFromSearch } from '../gitlab.utils';

suite('Test GitLab PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => {
function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
assert.deepStrictEqual(
getGitLabPullRequestIdentityValuesFromSearch(query),
prNumber == null
? undefined
: {
ownerAndRepo: ownerAndRepo,
prNumber: prNumber,
},
`${message} (${JSON.stringify(query)})`,
);
}

test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => {
t('full URL', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1', '1', 'eamodio/vscode-gitlens');
t(
'with suffix',
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1/files?diff=unified#hello',
'1',
'eamodio/vscode-gitlens',
);
t(
'with query',
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1?diff=unified#hello',
'1',
'eamodio/vscode-gitlens',
);

t(
'with anchor',
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1#hello',
'1',
'eamodio/vscode-gitlens',
);
t(
'a weird suffix',
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1-files',
'1',
'eamodio/vscode-gitlens',
);
t('numeric repo name', 'https://gitlab.com/sergeibbb/1/-/merge_requests/16', '16', 'sergeibbb/1');

t(
'no protocol with leading slash',
'/gitlab.com/sergeibbb/1/-/merge_requests/16?diff=unified',
'16',
'sergeibbb/1',
);
t('no protocol without leading slash', 'gitlab.com/sergeibbb/1/-/merge_requests/16/files', '16', 'sergeibbb/1');
});
test('no domain, should parse to ownerAndRepo and prNumber', () => {
t('with leading slash', '/sergeibbb/1/-/merge_requests/16#hello', '16', 'sergeibbb/1');
t(
'words in repo name',
'eamodio/vscode-gitlens/-/merge_requests/1?diff=unified#hello',
'1',
'eamodio/vscode-gitlens',
);
t('numeric repo name', 'sergeibbb/1/-/merge_requests/16/files', '16', 'sergeibbb/1');
});

test('domain vs. no domain', () => {
t(
'with anchor',
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1#hello/sergeibbb/1/-/merge_requests/16',
'1',
'eamodio/vscode-gitlens',
);
});

test('numbers', () => {
t('has "-/merge_requests/" fragment', '/-/merge_requests/16/files#hello', '16');
t('has "-/merge_requests/" fragment with double slash', '1//-/merge_requests/16?diff=unified#hello', '16');
t('with leading slash', '/16/files#hello', '16');
t('just a number', '16', '16');
t('with a hash', '#16', '16');
});

test('does not match', () => {
t('without leading slash', '16?diff=unified#hello', undefined);
t('with leading hash', '/#16/files#hello', undefined);
t('number is a part of a word', 'hello16', undefined);
t('number is a part of a word', '16hello', undefined);

t('GitHub', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16');

t('with a number', '1/16?diff=unified#hello', '16');
t('with a number and slash', '/1/16?diff=unified#hello', '1');
t('with a word', 'anything/16?diff=unified#hello', '16');

t(
'with a wrong character leading to "-/merge_requests/"',
'sergeibbb/1/--/merge_requests/16?diff=unified#hello',
'1',
);
t(
'with a wrong character leading to "-/merge_requests/"',
'sergeibbb/1--/merge_requests/16?diff=unified#hello',
'1',
);
});
});
6 changes: 6 additions & 0 deletions src/plus/integrations/providers/github.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import type {
PullRequestState,
SearchedPullRequest,
} from '../../../git/models/pullRequest';
import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils';
import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata';
import { log } from '../../../system/decorators/log';
import { ensurePaidPlan } from '../../utils';
Expand All @@ -20,6 +21,7 @@ import type {
} from '../authentication/integrationAuthentication';
import type { RepositoryDescriptor, SupportedIntegrationIds } from '../integration';
import { HostingIntegration } from '../integration';
import { getGitHubPullRequestIdentityValuesFromSearch } from './github.utils';
import { providersMetadata } from './models';
import type { ProvidersApi } from './providersApi';

Expand Down Expand Up @@ -292,6 +294,10 @@ export class GitHubIntegration extends GitHubIntegrationBase<HostingIntegrationI
super.refresh();
}
}

override getPullRequestIdentityValuesFromSearch(search: string): PullRequestUrlIdentity | undefined {
return getGitHubPullRequestIdentityValuesFromSearch(search);
}
}

export class GitHubEnterpriseIntegration extends GitHubIntegrationBase<SelfHostedIntegrationId.GitHubEnterprise> {
Expand Down
28 changes: 28 additions & 0 deletions src/plus/integrations/providers/github.utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// GitHub provider: github.ts pulls many dependencies through Container and some of them break the unit tests.
// That's why this file has been created that can collect more simple functions which
// don't require Container and can be tested.

import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils';
import { getPullRequestIdentityValuesFromSearch } from '../../../git/models/pullRequest.utils';

export function getGitHubPullRequestIdentityValuesFromSearch(search: string): PullRequestUrlIdentity | undefined {
let ownerAndRepo: string | undefined = undefined;
let prNumber: string | undefined = undefined;

let match = search.match(/([^/]+\/[^/]+)\/(?:pull)\/(\d+)/); // with org and rep name
if (match != null) {
ownerAndRepo = match[1];
prNumber = match[2];
}

if (prNumber == null) {
match = search.match(/(?:\/|^)(?:pull)\/(\d+)/); // without repo name
if (match != null) {
prNumber = match[1];
}
}

return prNumber != null
? { ownerAndRepo: ownerAndRepo, prNumber: prNumber }
: getPullRequestIdentityValuesFromSearch(search);
}
Loading

0 comments on commit 816809d

Please sign in to comment.