Skip to content

fix Deprecated Warning - #102

Open
dushibaiyu wants to merge 1 commit into
vert-x3:masterfrom
dushibaiyu:messageID
Open

fix Deprecated Warning#102
dushibaiyu wants to merge 1 commit into
vert-x3:masterfrom
dushibaiyu:messageID

Conversation

@dushibaiyu

Copy link
Copy Markdown
  • messageId
  • password
  • willMessage

@Sammers21

Copy link
Copy Markdown
Contributor

@dushibaiyu , sorry, I did not get it. Which warning are you trying to fix? Can you explain?

@Sammers21

Copy link
Copy Markdown
Contributor

@dushibaiyu , this PR also contains some parts from the #101. Can you remove it, so the PR will contain only original changes?

* messageId
* password
* willMessage
@dushibaiyu

Copy link
Copy Markdown
Author

@Sammers21 Ok. i have remove the #101 commit.

  • In class MqttPublishVariableHeader the messageId() is deprecated it return the packetId.
  • In class MqttConnectPayload the willMessage() and password() is deprecated .

@ppatierno ppatierno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With this PR I would just fix the deprecated warnings for messageId. I think that will message and password should have a different PR because ...
Since the first MQTT implementation in Netty, it was a big mistake having will message and password as String because they are just bytes in the MQTT specification. Now that it was addressed in Netty, I think that we should have a breaking change here having will message and password changing from String to bytes in the MQTT library as well. I know that Netty has is maintaining its back compatibility with fields like willMessage and willMessageInBytes with the first one having the bytes encoded in UTF-8 but the first one doesn't make sense if the will message is a really raw binary message with no meaning as String. Wdyt @vietj , is that bad having a breaking change here? I mean exposing changing from String to bytes without having both exposed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants