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

Class StompFrameMessage does not know which protocol version is used on the connection #1533

Closed
wants to merge 2 commits into from

Conversation

michaelJustin
Copy link
Contributor

@michaelJustin michaelJustin commented Jul 23, 2022

@pzygielo pzygielo changed the title 1524 Class StompFrameMessage does not know which protocol version is used on the connection Class StompFrameMessage does not know which protocol version is used on the connection Jul 23, 2022
@pzygielo pzygielo self-requested a review July 23, 2022 07:39
Copy link
Contributor

@pzygielo pzygielo left a comment

Choose a reason for hiding this comment

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

I'd prefer commit message to follow something like

rather than describe the bug given change intends to fix.

@@ -22,4 +22,8 @@
public interface StompFrameMessageFactory {

StompFrameMessage newStompFrameMessage(StompFrameMessage.Command cmd, LoggerWrapper logger);

void setVersion(String version);
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't feel right place for version. Factory does not use this, and only serves as transport for it. I'd need (more) time to investigate other possibilities (or less time to review other solution).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I see. The StompProtocolHandler already has the version private field, so this only needs to be exposed (using a getter). I will create a new PR. If I have done the fix for #1534, I will include it in the same PR.

@michaelJustin
Copy link
Contributor Author

I have created a new pull request (#1545)

@michaelJustin michaelJustin deleted the OMQ-1524 branch August 1, 2022 15:52
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