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

really small correction with url.join #181

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

1majom
Copy link
Contributor

@1majom 1majom commented Aug 12, 2024

I know this is not an important change, but I came across it while changing the api for my own project and it might help others who want to do the same.

The url's join method overwrites the previous join. So e.g. url.join("origin/").join("vaj") won't make an url like kiscica.hu/origin/vaj, but kiscica.hu/vaj.

@kixelated
Copy link
Owner

TIL URL::join is weird. But shouldn't the existing code work because of the trailing slash? That's probably why I added it in the first place.

@1majom
Copy link
Contributor Author

1majom commented Aug 16, 2024

The existing code will work because this is consistent, so for all paths it will be only moq.relay/ not moq.relay/origin/. I just wanted to correct this because when I made more complicated paths I saw that double joins won't get us the wanted outcome.

But if you prefer it that way, instead of sticking to the origin/ part, the affected lines could be replaced simply with "self.url.join(namespace)?". That way it will do the same as now without the double joins.

@kixelated
Copy link
Owner

I just wasn't sure if anything was broken. Thanks for the improvement!

@kixelated kixelated merged commit e6a92a8 into kixelated:main Aug 16, 2024
1 check passed
This was referenced Aug 16, 2024
@github-actions github-actions bot mentioned this pull request Sep 3, 2024
@github-actions github-actions bot mentioned this pull request Oct 1, 2024
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.

2 participants