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

MQTT Binding. #513

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

MQTT Binding. #513

wants to merge 2 commits into from

Conversation

JemDay
Copy link
Contributor

@JemDay JemDay commented Jan 18, 2023

For comment at present - DO NOT MERGE.

Contains MQTT V3 & V5 binding implementations for the Paho and HiveMQ client libraries.

Closes #335

@JemDay
Copy link
Contributor Author

JemDay commented Jan 18, 2023

@Alfusainey

@Alfusainey
Copy link
Contributor

@Alfusainey

@JemDay LGTM. This is a neat contribution, thanks!

@JemDay
Copy link
Contributor Author

JemDay commented Jan 26, 2023

Thanks @Alfusainey - I do need to clean up the documentation side of it but if there's anything else that appears glaringly wrong pls let me know.

@pierDipi - if you could start taking a peek at this I'd appreciate it.

Comment on lines 27 to 51
/**
* Get the default content type to assume for MQTT messages.
* @return A Content-Type
*/
public static final String getDefaultContentType() {
return DEFAULT_FORMAT;
}
Copy link
Member

Choose a reason for hiding this comment

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

what's rationale for using a public static method vs referencing the field directly and make it public?

Comment on lines 22 to 23
// No-Op
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Should we throw an exception in this case mostly for "internal safety"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need to do some general clean-up ..

As an aside do we know why the definition of a 'MessageWriter' interface is so complicated, I'm not sure why it needs to extend other writers and factories.

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

On multiple files the copyright header is missing:

/*
* Copyright 2018-Present The CloudEvents Authors
* <p>
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* <p>
* http://www.apache.org/licenses/LICENSE-2.0
* <p>
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

@pierDipi
Copy link
Member

btw, I'm still going through it

@JemDay
Copy link
Contributor Author

JemDay commented Mar 10, 2023

thx ... I'll try and take a look at this next week now that the proto changes are in ...

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.

MQTT protocol binding
3 participants