Skip to content

Commit 3a33267

Browse files
sergeibbbsergeibbb
committed
Performs specific PR URL match for each connected provider
(#3788, #3795)
1 parent 7863d5d commit 3a33267

File tree

15 files changed

+384
-207
lines changed

15 files changed

+384
-207
lines changed

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

Lines changed: 17 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,62 +1,30 @@
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+
provider: undefined,
15+
},
1316
`${message} (${JSON.stringify(query)})`,
1417
);
1518
}
1619

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-
});
20+
test('cannot recognize GitHub or GitLab URLs, sees only numbers', () => {
21+
t('full URL', 'https://github.com/eamodio/vscode-gitlens/pull/16', '16');
22+
t('numeric repo name', 'https://github.com/sergeibbb/1/pull/16', '1');
5423

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');
24+
t('no protocol', '/github.com/sergeibbb/1/pull/16?diff=unified', '1');
25+
t('no domain', '/sergeibbb/1/pull/16#hello', '1');
26+
t('domain vs. no domain', 'https://github.com/eamodio/vscode-gitlens/pull/1#hello/sergeibbb/2/pull/16', '1');
27+
t('has "pull/" fragment', '/pull/16/files#hello', '16');
6028
});
6129

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

‎src/git/models/pullRequest.ts‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,10 @@ import { Uri, window } from 'vscode';
22
import { Schemes } from '../../constants';
33
import { Container } from '../../container';
44
import type { RepositoryIdentityDescriptor } from '../../gk/models/repositoryIdentities';
5-
import type { EnrichablePullRequest } from '../../plus/integrations/providers/models';
65
import { formatDate, fromNow } from '../../system/date';
76
import { memoize } from '../../system/decorators/memoize';
87
import type { LeftRightCommitCountResult } from '../gitProvider';
98
import type { IssueOrPullRequest, IssueRepository, IssueOrPullRequestState as PullRequestState } from './issue';
10-
import type { PullRequestUrlIdentity } from './pullRequest.utils';
119
import type { ProviderReference } from './remoteProvider';
1210
import type { Repository } from './repository';
1311
import { createRevisionRange, shortenRevision } from './revision.utils';
@@ -419,21 +417,3 @@ export async function getOpenedPullRequestRepo(
419417
const repo = await getOrOpenPullRequestRepository(container, pr, { promptIfNeeded: true });
420418
return repo;
421419
}
422-
423-
export function doesPullRequestSatisfyRepositoryURLIdentity(
424-
pr: EnrichablePullRequest | undefined,
425-
{ ownerAndRepo, prNumber }: PullRequestUrlIdentity,
426-
): boolean {
427-
if (pr == null) {
428-
return false;
429-
}
430-
const satisfiesPrNumber = prNumber != null && pr.number === parseInt(prNumber, 10);
431-
if (!satisfiesPrNumber) {
432-
return false;
433-
}
434-
const satisfiesOwnerAndRepo = ownerAndRepo != null && pr.repoIdentity.name === ownerAndRepo;
435-
if (!satisfiesOwnerAndRepo) {
436-
return false;
437-
}
438-
return true;
439-
}

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

Lines changed: 9 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,31 +8,19 @@ 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 isMaybeNonSpecificPullRequestSearchUrl(search: string): boolean {
15+
return getPullRequestIdentityFromMaybeUrl(search) != null;
16+
}
17+
18+
export function getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
1619
let prNumber: string | undefined = undefined;
1720

18-
let match = search.match(/([^/]+\/[^/]+)\/(?:pull|-\/merge_requests)\/(\d+)/); // with org and rep name
21+
let match = search.match(/(?:\/)(\d+)/); // any number starting with "/"
1922
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-
}
23+
prNumber = match[1];
3624
}
3725

3826
if (prNumber == null) {
@@ -42,5 +30,5 @@ export function getPullRequestIdentityValuesFromSearch(search: string): PullRequ
4230
}
4331
}
4432

45-
return { ownerAndRepo: ownerAndRepo, prNumber: prNumber };
33+
return prNumber == null ? undefined : { ownerAndRepo: undefined, prNumber: prNumber, provider: undefined };
4634
}

‎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/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/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/__tests__/github.utils.test.ts‎

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

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
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 { HostingIntegrationId } from '../../../../constants.integrations';
6+
import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils';
7+
8+
export function isMaybeGitHubPullRequestUrl(url: string): boolean {
9+
if (url == null) return false;
10+
11+
return getGitHubPullRequestIdentityFromMaybeUrl(url) != null;
12+
}
13+
14+
export function getGitHubPullRequestIdentityFromMaybeUrl(
15+
search: string,
16+
): (PullRequestUrlIdentity & { provider: HostingIntegrationId.GitHub }) | undefined {
17+
let ownerAndRepo: string | undefined = undefined;
18+
let prNumber: string | undefined = undefined;
19+
20+
let match = search.match(/([^/]+\/[^/]+)\/(?:pull)\/(\d+)/); // with org and rep name
21+
if (match != null) {
22+
ownerAndRepo = match[1];
23+
prNumber = match[2];
24+
}
25+
26+
if (prNumber == null) {
27+
match = search.match(/(?:\/|^)(?:pull)\/(\d+)/); // without repo name
28+
if (match != null) {
29+
prNumber = match[1];
30+
}
31+
}
32+
33+
return prNumber != null
34+
? { ownerAndRepo: ownerAndRepo, prNumber: prNumber, provider: HostingIntegrationId.GitHub }
35+
: undefined;
36+
}

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

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
import type { Endpoints } from '@octokit/types';
2-
import { HostingIntegrationId } from '../../../../constants.integrations';
32
import { GitFileIndexStatus } from '../../../../git/models/file';
43
import type { IssueLabel } from '../../../../git/models/issue';
54
import { Issue, RepositoryAccessLevel } from '../../../../git/models/issue';
@@ -11,7 +10,6 @@ import {
1110
PullRequestReviewState,
1211
PullRequestStatusCheckRollupState,
1312
} from '../../../../git/models/pullRequest';
14-
import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils';
1513
import type { Provider } from '../../../../git/models/remoteProvider';
1614

1715
export interface GitHubBlame {
@@ -511,20 +509,3 @@ export function fromCommitFileStatus(
511509
}
512510
return undefined;
513511
}
514-
515-
const prUrlRegex = /^(?:https?:\/\/)?(?:github\.com\/)?([^/]+\/[^/]+)\/pull\/(\d+)/i;
516-
517-
export function isMaybeGitHubPullRequestUrl(url: string): boolean {
518-
if (url == null) return false;
519-
520-
return prUrlRegex.test(url);
521-
}
522-
523-
export function getGitHubPullRequestIdentityFromMaybeUrl(url: string): RequireSome<PullRequestUrlIdentity, 'provider'> {
524-
if (url == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitHub };
525-
526-
const match = prUrlRegex.exec(url);
527-
if (match == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitHub };
528-
529-
return { prNumber: match[2], ownerAndRepo: match[1], provider: HostingIntegrationId.GitHub };
530-
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import type {
1212
PullRequestState,
1313
SearchedPullRequest,
1414
} from '../../../git/models/pullRequest';
15+
import type { PullRequestUrlIdentity } from '../../../git/models/pullRequest.utils';
1516
import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata';
1617
import { log } from '../../../system/decorators/log';
1718
import { uniqueBy } from '../../../system/iterable';
@@ -22,6 +23,7 @@ import type {
2223
} from '../authentication/integrationAuthentication';
2324
import type { RepositoryDescriptor } from '../integration';
2425
import { HostingIntegration } from '../integration';
26+
import { getGitLabPullRequestIdentityFromMaybeUrl } from './gitlab/gitlab.utils';
2527
import { fromGitLabMergeRequestProvidersApi } from './gitlab/models';
2628
import type { ProviderPullRequest } from './models';
2729
import { ProviderPullRequestReviewState, providersMetadata, toSearchedIssue } from './models';
@@ -393,6 +395,10 @@ abstract class GitLabIntegrationBase<
393395
username: currentUser.username || undefined,
394396
};
395397
}
398+
399+
override getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
400+
return getGitLabPullRequestIdentityFromMaybeUrl(search);
401+
}
396402
}
397403

398404
export class GitLabIntegration extends GitLabIntegrationBase<HostingIntegrationId.GitLab> {

0 commit comments

Comments
 (0)