-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Searches by URL for self hosted GitHub and GitLab on Launchpad #3942
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0d463ae
to
261dc44
Compare
Now it doesn't harm users. Just search by URL doesn't work for self hosted GitHub and GitLab. That is why I suppose this PR can be merged after the release as a follow-up. I haven't changed the CHANGELOG yet, because if we decide to merge before the release, it will be covered by general notes of adding self-hosted GitLab and GitHub. Also, I've noticed that GitLab's and GitHub's methods are similar: they get a value without |
261dc44
to
e4ccda5
Compare
e4ccda5
to
dd4e3ca
Compare
dd4e3ca
to
54f2e07
Compare
54f2e07
to
cfa54e2
Compare
i've fixed the conflicts here and did some testing |
cfa54e2
to
25b7a5e
Compare
25b7a5e
to
643b569
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together. Just a few very minor suggestions, then it looks good to me after.
CHANGELOG.md
Outdated
@@ -162,6 +162,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p | |||
- Fixes [#3940](https://github.com/gitkraken/vscode-gitlens/issues/3940) - Commit Details: issue autolinks can disappear after enrichment | |||
- Fixes [#4035](https://github.com/gitkraken/vscode-gitlens/issues/4035) - Repo is lost when "Generate commit" is requested | |||
|
|||
#### Added | |||
|
|||
- Searches by URL for self hosted GitHub and GitLab on Launchpad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Searches by URL for self hosted GitHub and GitLab on Launchpad | |
- Adds the ability to search for GitHub Enterprise and GitLab Self-Managed pull requests by URL in the main step of Launchpad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
return getGitHubPullRequestIdentityFromMaybeUrl(url) != null; | ||
} | ||
|
||
export function getGitHubPullRequestIdentityFromMaybeUrl( | ||
search: string, | ||
): (PullRequestUrlIdentity & { provider: HostingIntegrationId.GitHub }) | undefined { | ||
): Omit<PullRequestUrlIdentity, 'provider'> | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we just updated this function to require, in addition to the search
parameter, an id
parameter of type GitHubRelatedIntegrationIds
? Then you don't need to add on the id afterwards in the integration fn and can directly call this with its id.
Both ways achieve the same effect, maybe just easier not to have to shallow-clone the object afterwards, and you don't need to apply Omit
on the return type either.
And I would say the same thing for getGitLabPullRequestIdentityFromMaybeUrl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axosoft-ramint
fixed
@@ -11,7 +10,7 @@ export function isMaybeGitLabPullRequestUrl(url: string): boolean { | |||
|
|||
export function getGitLabPullRequestIdentityFromMaybeUrl( | |||
search: string, | |||
): (PullRequestUrlIdentity & { provider: HostingIntegrationId.GitLab }) | undefined { | |||
): Omit<PullRequestUrlIdentity, 'provider'> | undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment on getGitHubPullRequestIdentityFromMaybeUrl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@axosoft-ramint
fixed
643b569
to
2148232
Compare
follow up of #3923 and #3934
Description
Checklist
Fixes $XXX -
orCloses #XXX -
prefix to auto-close the issue that your PR addresses