Skip to content
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

fix: improve git remoteUrl parsing #4147

Merged
merged 8 commits into from
Jun 24, 2024

Conversation

ndom91
Copy link
Contributor

@ndom91 ndom91 commented Jun 21, 2024

☕️ Reasoning

🧢 Changes

  • New dependency git-url-parse (https://github.com/IonicaBizau/git-url-parse) [~3.5kb minified + gzipped]
    • GitHub only supports a subset of the remote URL types that git supports, so maybe the manual parsing we had was sufficient for the time being
    • But having less error-prone / battle tested parsing will make it easier for us to support other git services in the future and avoid other edge-cases
  • Add a few initial vitest tests for the GitHubService to ensure that owner/repo are parsed out as expected

Out of the 22 URLs I added to test, the previous implementation only parsed 13/22 correctly.

Copy link
Collaborator

@Byron Byron left a comment

Choose a reason for hiding this comment

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

That's a great call, and of course the JS world has a parser already which probably covers 99% of the common cases as well by now.

So I'd say, despite my recommendation, let's use it until it breaks :).

import { expect, test, describe } from 'vitest';

const exampleRemoteUrls = [
'ssh://user@host.xz:123/org/repo.git/',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am loving this!

@Caleb-T-Owens
Copy link
Contributor

Love this @ndom91!!

If you're up for it, do you want to change convertRemoteToWebUrl to something like: GitUrlParse("...").toString("https").replace(/.git$/, '')?

@ndom91 ndom91 requested review from Caleb-T-Owens and removed request for Caleb-T-Owens June 24, 2024 15:29
@ndom91 ndom91 changed the title feat: add initial GitHubService test for parsing remote URLs fix: improve git remoteUrl parsing Jun 24, 2024
@ndom91 ndom91 merged commit d013b7b into master Jun 24, 2024
16 checks passed
@ndom91 ndom91 deleted the ndom91/add-githubservice-initial-test branch June 24, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants