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 double slash in url. #80

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

mstyura
Copy link
Contributor

@mstyura mstyura commented Mar 13, 2024

About the changes

It was unclear whether it was expected by user that provided API url is end with / or not. I've actually passed URL with trailing /, which ended up inability to register client. It wen't unnoticed until deployed on prod, because there was different URL.
image
I've made a change to make SDK tolerate URLs ended with / by trimming user-provided URL before concatenation with rest of endpoint.

Important files

Discussion points

@mstyura
Copy link
Contributor Author

mstyura commented Mar 14, 2024

@daveleek could you please take a look?

@mstyura
Copy link
Contributor Author

mstyura commented Mar 14, 2024

Copy link
Contributor

@thomasheartman thomasheartman left a comment

Choose a reason for hiding this comment

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

Hey, @mstyura! This seems like a very good change to me and I'm all for it! 🔥

Gonna double check internally, but I imagine we can just go ahead and merge it.

Thanks for the PR and good work on this 🙌🏼

src/api.rs Outdated Show resolved Hide resolved
@thomasheartman
Copy link
Contributor

Noticed Clippy is failing for a file you haven't touched. That should be perfectly safe to ignore, but I wanna see if I can make CI green before merging anything. I've linked the (merged) pull request that fixes it. Would you mind just rebasing / merging those changes so we can get the full pipeline to green?

@mstyura mstyura force-pushed the api-url-double-slash branch from 45c848b to 4632fde Compare March 14, 2024 16:27
@mstyura mstyura force-pushed the api-url-double-slash branch from 4632fde to 8b9935f Compare March 14, 2024 16:35
@thomasheartman thomasheartman merged commit d5e236c into Unleash:main Mar 15, 2024
8 checks passed
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