Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p

### Added

- Adds new ability to search for a GitLab MR in the _Launchpad_ — closes [#3788](https://github.com/gitkraken/vscode-gitlens/issues/3788)
- Adds go to home view button to the commit graph title section — closes [#3873](https://github.com/gitkraken/vscode-gitlens/issues/3873)
- Adds a _Contributors_ section to comparison results in the views

Expand Down
66 changes: 17 additions & 49 deletions src/git/models/__tests__/pullRequest.utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,62 +1,30 @@
import * as assert from 'assert';
import { suite, test } from 'mocha';
import { getPullRequestIdentityValuesFromSearch } from '../pullRequest.utils';
import { getPullRequestIdentityFromMaybeUrl } from '../pullRequest.utils';

suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityValuesFromSearch()', () => {
suite('Test PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => {
function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
assert.deepStrictEqual(
getPullRequestIdentityValuesFromSearch(query),
{
ownerAndRepo: ownerAndRepo,
prNumber: prNumber,
},
getPullRequestIdentityFromMaybeUrl(query),
prNumber == null
? undefined
: {
ownerAndRepo: ownerAndRepo,
prNumber: prNumber,
provider: undefined,
},
`${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
20 changes: 0 additions & 20 deletions src/git/models/pullRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@ import { Uri, window } from 'vscode';
import { Schemes } from '../../constants';
import { Container } from '../../container';
import type { RepositoryIdentityDescriptor } from '../../gk/models/repositoryIdentities';
import type { EnrichablePullRequest } from '../../plus/integrations/providers/models';
import { formatDate, fromNow } from '../../system/date';
import { memoize } from '../../system/decorators/memoize';
import type { LeftRightCommitCountResult } from '../gitProvider';
import type { IssueOrPullRequest, IssueRepository, IssueOrPullRequestState as PullRequestState } from './issue';
import type { PullRequestUrlIdentity } from './pullRequest.utils';
import type { ProviderReference } from './remoteProvider';
import type { Repository } from './repository';
import { createRevisionRange, shortenRevision } from './revision.utils';
Expand Down Expand Up @@ -419,21 +417,3 @@ export async function getOpenedPullRequestRepo(
const repo = await getOrOpenPullRequestRepository(container, pr, { promptIfNeeded: true });
return repo;
}

export function doesPullRequestSatisfyRepositoryURLIdentity(
pr: EnrichablePullRequest | undefined,
{ ownerAndRepo, prNumber }: PullRequestUrlIdentity,
): boolean {
if (pr == null) {
return false;
}
const satisfiesPrNumber = prNumber != null && pr.number === parseInt(prNumber, 10);
if (!satisfiesPrNumber) {
return false;
}
const satisfiesOwnerAndRepo = ownerAndRepo != null && pr.repoIdentity.name === ownerAndRepo;
if (!satisfiesOwnerAndRepo) {
return false;
}
return true;
}
30 changes: 9 additions & 21 deletions src/git/models/pullRequest.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,31 +8,19 @@ export interface PullRequestUrlIdentity {
provider?: HostingIntegrationId;

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

export function getPullRequestIdentityValuesFromSearch(search: string): PullRequestUrlIdentity {
let ownerAndRepo: string | undefined = undefined;
export function isMaybeNonSpecificPullRequestSearchUrl(search: string): boolean {
return getPullRequestIdentityFromMaybeUrl(search) != null;
}

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

let match = search.match(/([^/]+\/[^/]+)\/pull\/(\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\/(\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 +30,5 @@ export function getPullRequestIdentityValuesFromSearch(search: string): PullRequ
}
}

return { ownerAndRepo: ownerAndRepo, prNumber: prNumber };
return prNumber == null ? undefined : { ownerAndRepo: undefined, prNumber: prNumber, provider: undefined };
}
9 changes: 8 additions & 1 deletion 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 @@ -556,7 +557,7 @@ export abstract class IntegrationBase<
const connected = this.maybeConnected ?? (await this.isConnected());
if (!connected) return undefined;

const pr = this.container.cache.getPullRequest(id, resource, this, () => ({
const pr = await this.container.cache.getPullRequest(id, resource, this, () => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unnecessary since we return the promise in the next line, similar to other cases. If anything, I would suggest instead that we just combine the two lines to return this.container.cache.get... and do the same in other places where this pattern occurs across the file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@axosoft-ramint

I did it because I set a breakpoint at this place it was convenient to see a resolved promise.

value: (async () => {
try {
const result = await this.getProviderPullRequest?.(this._session!, resource, id);
Expand Down Expand Up @@ -1361,4 +1362,10 @@ export abstract class HostingIntegration<
repos?: T[],
cancellation?: CancellationToken,
): Promise<PullRequest[] | undefined>;

getPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
return this.getProviderPullRequestIdentityFromMaybeUrl?.(search);
}

protected getProviderPullRequestIdentityFromMaybeUrl?(search: string): PullRequestUrlIdentity | undefined;
}
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 { getGitHubPullRequestIdentityFromMaybeUrl } from './github/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();
}
}

protected override getProviderPullRequestIdentityFromMaybeUrl(search: string): PullRequestUrlIdentity | undefined {
return getGitHubPullRequestIdentityFromMaybeUrl(search);
}
}

export class GitHubEnterpriseIntegration extends GitHubIntegrationBase<SelfHostedIntegrationId.GitHubEnterprise> {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import * as assert from 'assert';
import { suite, test } from 'mocha';
//import { getGitHubPullRequestIdentityFromMaybeUrl } from '../github/models';
import { getGitHubPullRequestIdentityFromMaybeUrl, isMaybeGitHubPullRequestUrl } from '../github.utils';

suite('Test GitHub PR URL parsing to identity: getPullRequestIdentityFromMaybeUrl()', () => {
function t(message: string, query: string, prNumber: string | undefined, ownerAndRepo?: string) {
assert.deepStrictEqual(
getGitHubPullRequestIdentityFromMaybeUrl(query),
prNumber == null
? undefined
: {
ownerAndRepo: ownerAndRepo,
prNumber: prNumber,
provider: 'github',
},
`Parse: ${message} (${JSON.stringify(query)})`,
);
assert.equal(
isMaybeGitHubPullRequestUrl(query),
prNumber != null,
`Check: ${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', undefined);
t('just a number', '16', undefined);
t('with a hash', '#16', undefined);
});

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', undefined);

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

t('with a wrong character leading to pull', 'sergeibbb/1/-pull/16?diff=unified#hello', undefined);
t('with a wrong character leading to pull', 'sergeibbb/1-pull/16?diff=unified#hello', undefined);
});
});
36 changes: 36 additions & 0 deletions src/plus/integrations/providers/github/github.utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// 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 { HostingIntegrationId } from '../../../../constants.integrations';
import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils';

export function isMaybeGitHubPullRequestUrl(url: string): boolean {
if (url == null) return false;

return getGitHubPullRequestIdentityFromMaybeUrl(url) != null;
}

export function getGitHubPullRequestIdentityFromMaybeUrl(
search: string,
): (PullRequestUrlIdentity & { provider: HostingIntegrationId.GitHub }) | 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, provider: HostingIntegrationId.GitHub }
: undefined;
}
19 changes: 0 additions & 19 deletions src/plus/integrations/providers/github/models.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { Endpoints } from '@octokit/types';
import { HostingIntegrationId } from '../../../../constants.integrations';
import { GitFileIndexStatus } from '../../../../git/models/file';
import type { IssueLabel } from '../../../../git/models/issue';
import { Issue, RepositoryAccessLevel } from '../../../../git/models/issue';
Expand All @@ -11,7 +10,6 @@ import {
PullRequestReviewState,
PullRequestStatusCheckRollupState,
} from '../../../../git/models/pullRequest';
import type { PullRequestUrlIdentity } from '../../../../git/models/pullRequest.utils';
import type { Provider } from '../../../../git/models/remoteProvider';

export interface GitHubBlame {
Expand Down Expand Up @@ -511,20 +509,3 @@ export function fromCommitFileStatus(
}
return undefined;
}

const prUrlRegex = /^(?:https?:\/\/)?(?:github\.com\/)?([^/]+\/[^/]+)\/pull\/(\d+)/i;

export function isMaybeGitHubPullRequestUrl(url: string): boolean {
if (url == null) return false;

return prUrlRegex.test(url);
}

export function getGitHubPullRequestIdentityFromMaybeUrl(url: string): RequireSome<PullRequestUrlIdentity, 'provider'> {
if (url == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitHub };

const match = prUrlRegex.exec(url);
if (match == null) return { prNumber: undefined, ownerAndRepo: undefined, provider: HostingIntegrationId.GitHub };

return { prNumber: match[2], ownerAndRepo: match[1], provider: HostingIntegrationId.GitHub };
}
Loading
Loading