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

Docs: instance.publish(packet) expects an aedes-packet object #70

Merged
merged 2 commits into from
Oct 5, 2016
Merged

Docs: instance.publish(packet) expects an aedes-packet object #70

merged 2 commits into from
Oct 5, 2016

Conversation

eladnava
Copy link
Contributor

@eladnava eladnava commented Oct 3, 2016

Updated the docs to match the new parameter expectation.

… an mqtt-packet

Updated the docs to match the new parameter expectation.
@coveralls
Copy link

coveralls commented Oct 3, 2016

Coverage Status

Coverage remained the same at 97.036% when pulling caca338 on eladnava:patch-1 into 8dd6485 on mcollina:master.

@@ -143,7 +143,7 @@ It supports backpressure.
### instance.publish(packet, done)

Publish the given packet to subscribed clients and functions. A packet
must be valid for [mqtt-packet](http://npm.im/mqtt-packet).
must be valid for [aedes-packet](http://npm.im/aedes-packet).
Copy link
Collaborator

Choose a reason for hiding this comment

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

In aedes-packet, everything is defaulted:

https://github.com/mcollina/aedes-packet/blob/master/packet.js#L4-L11.

I think a much better definition can be: a packet must have the following properties...

Copy link
Contributor Author

@eladnava eladnava Oct 4, 2016

Choose a reason for hiding this comment

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

@mcollina I think you have outlined it well here:
https://www.npmjs.com/package/mqtt-packet#publish

Should I just copy over that explanation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so, yes!

Copy link
Contributor Author

@eladnava eladnava Oct 5, 2016

Choose a reason for hiding this comment

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

@mcollina Updated the PR 😄 Let me know if anything is missing.

@coveralls
Copy link

coveralls commented Oct 5, 2016

Coverage Status

Coverage remained the same at 97.036% when pulling ad0e0f2 on eladnava:patch-1 into 8dd6485 on mcollina:master.

@mcollina mcollina merged commit 285dd87 into moscajs:master Oct 5, 2016
@mcollina
Copy link
Collaborator

mcollina commented Oct 5, 2016

Perfect, thanks!

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