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

Support OTP 25 #73

Merged
merged 1 commit into from
Sep 21, 2022
Merged

Support OTP 25 #73

merged 1 commit into from
Sep 21, 2022

Conversation

kelcecil
Copy link

@kelcecil kelcecil commented Jun 8, 2022

This MR replaces http_uri.parse/1 (removed in OTP 25) with uri_string.parse/1 (introduced in OTP 21). Additionally, this also expands the tested OTP versions in Travis CI to include OTP 23, 24, and 25 to ensure the use of uri_string.parse/1 doesn't break anything. Fixes #72.

@kelcecil
Copy link
Author

kelcecil commented Jun 8, 2022

On further observation, there's another nuance I need to deal with here. uri_string:parse/1 returns a map while http_uri:parse/1 returns a tuple. I can make this adjustment later today or tomorrow morning.

@kelcecil kelcecil force-pushed the support-otp-25 branch 2 times, most recently from 956f1c4 to fc9c76b Compare June 9, 2022 16:17
@kelcecil
Copy link
Author

kelcecil commented Jun 9, 2022

I've made my adjustments and have the test suite passing. I think this is ready for consideration.

@yuriploc
Copy link

Thank you, @kelcecil. This will help us keep moving forward :)

@kelcecil
Copy link
Author

Bumping this thread. This issue prevents our upgrade to OTP 25, and I'd love to get it merged to avoid managing our own fork. Let me know what I can do!

@sanmiguel
Copy link
Owner

Hi @kelcecil - thanks so much for this! I'll hopefully get a chance to check it out early next week, will let you know when I get to it.

@fendent
Copy link

fendent commented Jul 27, 2022

Looking forward to seeing this get merged in. Thanks for the work!

@kelcecil
Copy link
Author

kelcecil commented Aug 3, 2022

Hey! Just checking in! Any fixes/changes/suggestions I need to apply for this to be merged? Happy to do what we need to move this forward!

@sanmiguel
Copy link
Owner

Hi, thanks so much for your patience.

I've had to migrate to Github Actions for CI - would you mind rebasing and adding 25.0 to the matrix here to trigger a test run with your changes?

Thanks again for your patience, and for your contribution 👍 🙏

This updates websocket_client.start_link/5 to use uri_string.parse/1
instead of http_uri.parse/1. http_uri.parse/1 was removed in OTP 25
and is recommended to be repalced with uri_string.parse/1 which was
introduced in OTP 21.
@kelcecil
Copy link
Author

kelcecil commented Aug 5, 2022

Absolutely! Added and waiting for approval to run!

@kelcecil
Copy link
Author

Following up again. Anything I can do to help move this forward?

@crenwick
Copy link

Love all the work on this! We have a branch pointing mix to the github branch but I'd love to call it stable use hex when this is merged.

Any idea when that might be?

@sanmiguel
Copy link
Owner

Apologies for the delay. I've been trying to get this to run CI against all the OTP versions in the matrix via Github Actions, but I only ever get a few minutes at a time to look at it.
I'm giving up on that as it just doesn't seem to work from PR branches in forks.

I'll merge it as-is, which will trigger CI to run against master. If that all passes I'll push it to hex.pm as a new tag.
If something goes wrong either we can iterate from there or back it out.

@sanmiguel sanmiguel merged commit 9088737 into sanmiguel:master Sep 21, 2022
@sanmiguel
Copy link
Owner

🎉
Thank you so much for your contribution and your patience.

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.

Support OTP 25
5 participants