Skip to content

Commit e96c6b1

Browse files
sergeibbbsergeibbb
committed
Groups repo+prID pairs by provider to perform the more precise search
(#3788, #3795)
1 parent fc8a601 commit e96c6b1

File tree

14 files changed

+367
-128
lines changed

14 files changed

+367
-128
lines changed

‎src/git/models/__tests__/pullRequest.utils.test.ts‎

Lines changed: 16 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,62 +1,29 @@
11
import * as assert from 'assert';
22
import { suite, test } from 'mocha';
3-
import { getPullRequestIdentityValuesFromSearch } from '../pullRequest.utils';
3+
import { getPullRequestIdentityFromMaybeUrl } from '../pullRequest.utils';
44

5-
suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => {
5+
suite('Test PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => {
66
function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
77
assert.deepStrictEqual(
8-
getPullRequestIdentityValuesFromSearch(query),
9-
{
10-
ownerAndRepo: ownerAndRepo,
11-
prNumber: prNumber,
12-
},
8+
getPullRequestIdentityFromMaybeUrl(query),
9+
prNumber == null
10+
? undefined
11+
: {
12+
ownerAndRepo: ownerAndRepo,
13+
prNumber: prNumber,
14+
},
1315
`${message} (${JSON.stringify(query)})`,
1416
);
1517
}
1618

17-
test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => {
18-
t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens');
19-
t(
20-
'with suffix',
21-
'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello',
22-
'1',
23-
'eamodio/vscode-gitlens',
24-
);
25-
t(
26-
'with query',
27-
'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello',
28-
'1',
29-
'eamodio/vscode-gitlens',
30-
);
31-
32-
t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens');
33-
t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens');
34-
t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1');
35-
36-
t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1');
37-
t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
38-
});
39-
40-
test('no domain, should parse to ownerAndRepo and prNumber', () => {
41-
t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1');
42-
t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens');
43-
t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
44-
});
45-
46-
test('domain vs. no domain', () => {
47-
t(
48-
'with anchor',
49-
'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16',
50-
'1',
51-
'eamodio/vscode-gitlens',
52-
);
53-
});
19+
test('cannot recognize GitHub or GitLab URLs, sees only numbers', () => {
20+
t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16');
21+
t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '1');
5422

55-
test('has "pull/" fragment', () => {
56-
t('with leading slash', '/pull/16/files#hello', '16');
57-
t('without leading slash', 'pull/16?diff=unified#hello', '16');
58-
t('with numeric repo name', '1/pull/16?diff=unified#hello', '16');
59-
t('with double slash', '1//pull/16?diff=unified#hello', '16');
23+
t('no protocol', '/github.com/sergeibbb/1/pull/16?diff=unified', '1');
24+
t('no domain', '/sergeibbb/1/pull/16#hello', '1');
25+
t('domain vs. no domain', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/2/pull/16', '1');
26+
t('has "pull/" fragment', '/pull/16/files#hello', '16');
6027
});
6128

6229
test('has "/<num>" fragment', () => {

‎src/git/models/pullRequest.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -420,11 +420,12 @@ export async function getOpenedPullRequestRepo(
420420

421421
export function doesPullRequestSatisfyRepositoryURLIdentity(
422422
pr: EnrichablePullRequest | undefined,
423-
{ ownerAndRepo, prNumber }: PullRequestUrlIdentity,
423+
prUrlIdentity: { [key in string]?: PullRequestUrlIdentity },
424424
): boolean {
425425
if (pr == null) {
426426
return false;
427427
}
428+
const { ownerAndRepo, prNumber } = prUrlIdentity[pr.provider.id] ?? {};
428429
const satisfiesPrNumber = prNumber != null && pr.number === parseInt(prNumber, 10);
429430
if (!satisfiesPrNumber) {
430431
return false;

‎src/git/models/pullRequest.utils.ts‎

Lines changed: 6 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,31 +8,16 @@ export interface PullRequestUrlIdentity {
88
provider?: HostingIntegrationId;
99

1010
ownerAndRepo?: string;
11-
prNumber?: string;
11+
prNumber: string;
1212
}
1313

14-
export function getPullRequestIdentityValuesFromSearch(search: string): PullRequestUrlIdentity {
15-
let ownerAndRepo: string | undefined = undefined;
14+
export function getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
15+
const ownerAndRepo: string | undefined = undefined;
1616
let prNumber: string | undefined = undefined;
1717

18-
let match = search.match(/([^/]+\/[^/]+)\/(?:pull|-\/merge_requests)\/(\d+)/); // with org and rep name
18+
let match = search.match(/(?:\/)(\d+)/); // any number starting with "/"
1919
if (match != null) {
20-
ownerAndRepo = match[1];
21-
prNumber = match[2];
22-
}
23-
24-
if (prNumber == null) {
25-
match = search.match(/(?:\/|^)(?:pull|-\/merge_requests)\/(\d+)/); // without repo name
26-
if (match != null) {
27-
prNumber = match[1];
28-
}
29-
}
30-
31-
if (prNumber == null) {
32-
match = search.match(/(?:\/)(\d+)/); // any number starting with "/"
33-
if (match != null) {
34-
prNumber = match[1];
35-
}
20+
prNumber = match[1];
3621
}
3722

3823
if (prNumber == null) {
@@ -42,5 +27,5 @@ export function getPullRequestIdentityValuesFromSearch(search: string): PullRequ
4227
}
4328
}
4429

45-
return { ownerAndRepo: ownerAndRepo, prNumber: prNumber };
30+
return prNumber == null ? undefined : { ownerAndRepo: ownerAndRepo, prNumber: prNumber };
4631
}

‎src/plus/integrations/integration.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import type {
1616
PullRequestState,
1717
SearchedPullRequest,
1818
} from '../../git/models/pullRequest';
19+
import type { PullRequestUrlIdentity } from '../../git/models/pullRequest.utils';
1920
import type { RepositoryMetadata } from '../../git/models/repositoryMetadata';
2021
import { showIntegrationDisconnectedTooManyFailedRequestsWarningMessage } from '../../messages';
2122
import { gate } from '../../system/decorators/gate';
@@ -1361,4 +1362,6 @@ export abstract class HostingIntegration<
13611362
repos?: T[],
13621363
cancellation?: CancellationToken,
13631364
): Promise<PullRequest[] | undefined>;
1365+
1366+
getPullRequestIdentityFromMaybeUrl?(search: string): PullRequestUrlIdentity | undefined;
13641367
}

‎src/plus/integrations/providers/__tests__/github.utils.test.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
import * as assert from 'assert';
2+
import { suite, test } from 'mocha';
3+
import { getGitHubPullRequestIdentityFromMaybeUrl } from '../github.utils';
4+
5+
suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => {
6+
function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
7+
assert.deepStrictEqual(
8+
getGitHubPullRequestIdentityFromMaybeUrl(query),
9+
prNumber == null
10+
? undefined
11+
: {
12+
ownerAndRepo: ownerAndRepo,
13+
prNumber: prNumber,
14+
},
15+
`${message} (${JSON.stringify(query)})`,
16+
);
17+
}
18+
19+
test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => {
20+
t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/1', '1', 'eamodio/vscode-gitlens');
21+
t(
22+
'with suffix',
23+
'https://github.com/eamodio/vscode-gitlens/pull/1/files?diff=unified#hello',
24+
'1',
25+
'eamodio/vscode-gitlens',
26+
);
27+
t(
28+
'with query',
29+
'https://github.com/eamodio/vscode-gitlens/pull/1?diff=unified#hello',
30+
'1',
31+
'eamodio/vscode-gitlens',
32+
);
33+
34+
t('with anchor', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello', '1', 'eamodio/vscode-gitlens');
35+
t('a weird suffix', 'https://github.com/eamodio/vscode-gitlens/pull/1-files', '1', 'eamodio/vscode-gitlens');
36+
t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '16', 'sergeibbb/1');
37+
38+
t('no protocol with leading slash', '/github.com/sergeibbb/1/pull/16?diff=unified', '16', 'sergeibbb/1');
39+
t('no protocol without leading slash', 'github.com/sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
40+
});
41+
42+
test('no domain, should parse to ownerAndRepo and prNumber', () => {
43+
t('with leading slash', '/sergeibbb/1/pull/16#hello', '16', 'sergeibbb/1');
44+
t('words in repo name', 'eamodio/vscode-gitlens/pull/1?diff=unified#hello', '1', 'eamodio/vscode-gitlens');
45+
t('numeric repo name', 'sergeibbb/1/pull/16/files', '16', 'sergeibbb/1');
46+
});
47+
48+
test('domain vs. no domain', () => {
49+
t(
50+
'with anchor',
51+
'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/1/pull/16',
52+
'1',
53+
'eamodio/vscode-gitlens',
54+
);
55+
});
56+
57+
test('numbers', () => {
58+
t('has "pull/" fragment', '/pull/16/files#hello', '16');
59+
t('has "pull/" fragment with double slash', '1//pull/16?diff=unified#hello', '16');
60+
t('with leading slash', '/16/files#hello', '16');
61+
t('just a number', '16', '16');
62+
t('with a hash', '#16', '16');
63+
});
64+
65+
test('does not match', () => {
66+
t('without leading slash', '16?diff=unified#hello', undefined);
67+
t('with leading hash', '/#16/files#hello', undefined);
68+
t('number is a part of a word', 'hello16', undefined);
69+
t('number is a part of a word', '16hello', undefined);
70+
71+
t('GitLab', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/16', '16');
72+
73+
t('with a number', '1/16?diff=unified#hello', '16');
74+
t('with a number and slash', '/1/16?diff=unified#hello', '1');
75+
t('with a word', 'anything/16?diff=unified#hello', '16');
76+
77+
t('with a wrong character leading to pull', 'sergeibbb/1/-pull/16?diff=unified#hello', '1');
78+
t('with a wrong character leading to pull', 'sergeibbb/1-pull/16?diff=unified#hello', '1');
79+
});
80+
});

‎src/plus/integrations/providers/__tests__/gitlab.utils.test.ts‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
import * as assert from 'assert';
2+
import { suite, test } from 'mocha';
3+
import { getGitLabPullRequestIdentityFromMaybeUrl } from '../gitlab.utils';
4+
5+
suite('Test GitLab PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => {
6+
function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
7+
assert.deepStrictEqual(
8+
getGitLabPullRequestIdentityFromMaybeUrl(query),
9+
prNumber == null
10+
? undefined
11+
: {
12+
ownerAndRepo: ownerAndRepo,
13+
prNumber: prNumber,
14+
},
15+
`${message} (${JSON.stringify(query)})`,
16+
);
17+
}
18+
19+
test('full URL or without protocol but with domain, should parse to ownerAndRepo and prNumber', () => {
20+
t('full URL', 'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1', '1', 'eamodio/vscode-gitlens');
21+
t(
22+
'with suffix',
23+
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1/files?diff=unified#hello',
24+
'1',
25+
'eamodio/vscode-gitlens',
26+
);
27+
t(
28+
'with query',
29+
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1?diff=unified#hello',
30+
'1',
31+
'eamodio/vscode-gitlens',
32+
);
33+
34+
t(
35+
'with anchor',
36+
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1#hello',
37+
'1',
38+
'eamodio/vscode-gitlens',
39+
);
40+
t(
41+
'a weird suffix',
42+
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1-files',
43+
'1',
44+
'eamodio/vscode-gitlens',
45+
);
46+
t('numeric repo name', 'https://gitlab.com/sergeibbb/1/-/merge_requests/16', '16', 'sergeibbb/1');
47+
48+
t(
49+
'no protocol with leading slash',
50+
'/gitlab.com/sergeibbb/1/-/merge_requests/16?diff=unified',
51+
'16',
52+
'sergeibbb/1',
53+
);
54+
t('no protocol without leading slash', 'gitlab.com/sergeibbb/1/-/merge_requests/16/files', '16', 'sergeibbb/1');
55+
});
56+
test('no domain, should parse to ownerAndRepo and prNumber', () => {
57+
t('with leading slash', '/sergeibbb/1/-/merge_requests/16#hello', '16', 'sergeibbb/1');
58+
t(
59+
'words in repo name',
60+
'eamodio/vscode-gitlens/-/merge_requests/1?diff=unified#hello',
61+
'1',
62+
'eamodio/vscode-gitlens',
63+
);
64+
t('numeric repo name', 'sergeibbb/1/-/merge_requests/16/files', '16', 'sergeibbb/1');
65+
});
66+
67+
test('domain vs. no domain', () => {
68+
t(
69+
'with anchor',
70+
'https://gitlab.com/eamodio/vscode-gitlens/-/merge_requests/1#hello/sergeibbb/1/-/merge_requests/16',
71+
'1',
72+
'eamodio/vscode-gitlens',
73+
);
74+
});
75+
76+
test('numbers', () => {
77+
t('has "-/merge_requests/" fragment', '/-/merge_requests/16/files#hello', '16');
78+
t('has "-/merge_requests/" fragment with double slash', '1//-/merge_requests/16?diff=unified#hello', '16');
79+
t('with leading slash', '/16/files#hello', '16');
80+
t('just a number', '16', '16');
81+
t('with a hash', '#16', '16');
82+
});
83+
84+
test('does not match', () => {
85+
t('without leading slash', '16?diff=unified#hello', undefined);
86+
t('with leading hash', '/#16/files#hello', undefined);
87+
t('number is a part of a word', 'hello16', undefined);
88+
t('number is a part of a word', '16hello', undefined);
89+
90+
t('GitHub', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16');
91+
92+
t('with a number', '1/16?diff=unified#hello', '16');
93+
t('with a number and slash', '/1/16?diff=unified#hello', '1');
94+
t('with a word', 'anything/16?diff=unified#hello', '16');
95+
96+
t(
97+
'with a wrong character leading to "-/merge_requests/"',
98+
'sergeibbb/1/--/merge_requests/16?diff=unified#hello',
99+
'1',
100+
);
101+
t(
102+
'with a wrong character leading to "-/merge_requests/"',
103+
'sergeibbb/1--/merge_requests/16?diff=unified#hello',
104+
'1',
105+
);
106+
});
107+
});

‎src/plus/integrations/providers/github.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import type {
1111
PullRequestState,
1212
SearchedPullRequest,
1313
} from '../../../git/models/pullRequest';
14+
import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils';
1415
import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata';
1516
import { log } from '../../../system/decorators/log';
1617
import { ensurePaidPlan } from '../../utils';
@@ -20,6 +21,7 @@ import type {
2021
} from '../authentication/integrationAuthentication';
2122
import type { RepositoryDescriptor, SupportedIntegrationIds } from '../integration';
2223
import { HostingIntegration } from '../integration';
24+
import { getGitHubPullRequestIdentityFromMaybeUrl } from './github.utils';
2325
import { providersMetadata } from './models';
2426
import type { ProvidersApi } from './providersApi';
2527

@@ -292,6 +294,10 @@ export class GitHubIntegration extends GitHubIntegrationBase<HostingIntegrationI
292294
super.refresh();
293295
}
294296
}
297+
298+
override getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
299+
return getGitHubPullRequestIdentityFromMaybeUrl(search);
300+
}
295301
}
296302

297303
export class GitHubEnterpriseIntegration extends GitHubIntegrationBase<SelfHostedIntegrationId.GitHubEnterprise> {

‎src/plus/integrations/providers/github.utils.ts‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// GitHub provider: github.ts pulls many dependencies through Container and some of them break the unit tests.
2+
// That's why this file has been created that can collect more simple functions which
3+
// don't require Container and can be tested.
4+
5+
import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils';
6+
import { getPullRequestIdentityFromMaybeUrl } from '../../../git/models/pullRequest.utils';
7+
8+
export function getGitHubPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
9+
let ownerAndRepo: string | undefined = undefined;
10+
let prNumber: string | undefined = undefined;
11+
12+
let match = search.match(/([^/]+\/[^/]+)\/(?:pull)\/(\d+)/); // with org and rep name
13+
if (match != null) {
14+
ownerAndRepo = match[1];
15+
prNumber = match[2];
16+
}
17+
18+
if (prNumber == null) {
19+
match = search.match(/(?:\/|^)(?:pull)\/(\d+)/); // without repo name
20+
if (match != null) {
21+
prNumber = match[1];
22+
}
23+
}
24+
25+
return prNumber != null
26+
? { ownerAndRepo: ownerAndRepo, prNumber: prNumber }
27+
: getPullRequestIdentityFromMaybeUrl(search);
28+
}

0 commit comments

Comments
 (0)