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

upgrade to nom7 #13

Merged
merged 3 commits into from
Dec 30, 2023
Merged

upgrade to nom7 #13

merged 3 commits into from
Dec 30, 2023

Conversation

zuisong
Copy link
Contributor

@zuisong zuisong commented Dec 12, 2023

  • upgrade to nom7
  • distinguish between host and server address.

In the STOMP protocol specification, "host" and "server-address" are different concepts.

host : The name of a virtual host that the client wishes to connect to. It is recommended clients set this to the host name that the socket was established against, or to any name of their choosing. If this header does not match a known virtual host, servers supporting virtual hosting MAY select a default virtual host or reject the connection.

@zuisong zuisong mentioned this pull request Dec 12, 2023
@zuisong zuisong changed the title move to nom7 upgrade to nom7 Dec 12, 2023
src/client.rs Outdated Show resolved Hide resolved
@snaggen
Copy link

snaggen commented Dec 12, 2023

I have made some tests sending messages, and it seems to work as expected! I realize that I don't have any applications in rust that listens to messages, so I have not tested that.

@snaggen
Copy link

snaggen commented Dec 13, 2023

@zuisong I suggest that we, in the documentation for connect, mention something like. "If no specific virtualhost is desired, it is recommended to set this to the same as the host name that the socket was established against (i.e, the same as the server address)."

@snaggen
Copy link

snaggen commented Dec 13, 2023

Looks good. I have had this running on a system for the last day with no issues.

@alexkunde
Copy link
Owner

hey if you feel like taking over this project I would be glad to transfer it. it was useful for a while, but as you can see I don't really have time for it anymore.

@zuisong
Copy link
Contributor Author

zuisong commented Dec 14, 2023

Thank you for offering me the opportunity, but I'm sorry, I don't have any intention to take over this project at the moment. I hope you can find someone suitable to continue maintaining it. If you have any other questions or need assistance, I would be happy to help.

@alexkunde
Copy link
Owner

Thank you, i'll try to make some time to go over the changes!

@alexkunde
Copy link
Owner

@zuisong can you please run rust fmt on your fork? Ill restart the pipeline after

Copy link

codecov bot commented Dec 26, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (170d612) 60.17% compared to head (008ae55) 59.86%.
Report is 1 commits behind head on main.

Files Patch % Lines
src/client.rs 0.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
- Coverage   60.17%   59.86%   -0.31%     
==========================================
  Files           3        3              
  Lines         585      578       -7     
==========================================
- Hits          352      346       -6     
+ Misses        233      232       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor Author

@zuisong zuisong left a comment

Choose a reason for hiding this comment

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

@alexkunde
I have fixed the code. Can you please rerun the pipeline?

@alexkunde alexkunde merged commit 8bc7416 into alexkunde:main Dec 30, 2023
3 of 4 checks passed
@alexkunde
Copy link
Owner

Thank you for contributing, especially for moving this to nom7!

@snaggen snaggen mentioned this pull request Dec 31, 2023
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.

3 participants