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

GitHubRepoURL value type #65

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

Mr-Kanister
Copy link
Contributor

@Mr-Kanister Mr-Kanister commented Feb 15, 2025

This issue closes #28 .

The value type accepts a URL with the following constraints as input:

  • protocol: http or https
  • hostname: www.github.com or github.com
  • pathname: structure of a repo, i. e. /abc/def

SSH git URLs are unsupported as of now. If those should be considered in
the future, the class structure probably has to be modified.

How can this be tested?

npm run test

Screenshots

No UI changes made.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have lowered the linter errors. Before: 18. After: 16

@CLAassistant
Copy link

CLAassistant commented Feb 15, 2025

CLA assistant check
All committers have signed the CLA.

joluj
joluj previously approved these changes Feb 17, 2025
Copy link
Contributor

@joluj joluj left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Looks good

@joluj
Copy link
Contributor

joluj commented Feb 17, 2025

The CI fails, can you take a look into that?

@Mr-Kanister
Copy link
Contributor Author

Mr-Kanister commented Feb 17, 2025

I changed my url import to node:url, lets see if that works. Locally it doesn't make a difference. I fixed the missing user status transitions.

Edit: Well, I don't know why the CI doesn't know url

@Mr-Kanister Mr-Kanister requested a review from joluj February 17, 2025 10:14
@joluj
Copy link
Contributor

joluj commented Feb 17, 2025

The error comes up because node 20 (used in the CI) does not have the url module. I fixed this in #66. It should work after that one is merged and you've merged the main into this branch.

@joluj
Copy link
Contributor

joluj commented Feb 17, 2025

And it's merged, so please merge the main branch into your branch. Then it should work.

The value type accepts a URL with the following constraints as input:
- protocol: `http` or `https`
- hostname: `www.github.com` or `github.com`
- pathname: structure of a repo, i. e. `/abc/def`

SSH git URLs are unsupported as of now. If those should be considered in
the future, the class structure probably has to be modified.

Signed-off-by: Mr-Kanister <[email protected]>
@Mr-Kanister Mr-Kanister force-pushed the 28-githubrepourl-value-type branch from d2c7a5c to a1f4729 Compare February 17, 2025 10:41
Copy link
Contributor

@joluj joluj left a comment

Choose a reason for hiding this comment

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

Thank you!

@joluj joluj merged commit 17b0822 into riehlegroup:main Feb 17, 2025
7 checks passed
@Mr-Kanister Mr-Kanister deleted the 28-githubrepourl-value-type branch February 17, 2025 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create GitHubRepoURL value type
3 participants