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

Handling for other return codes #62

Merged
merged 9 commits into from
Aug 3, 2016
Merged

Conversation

GavinDmello
Copy link
Collaborator

This has the extra error codes Aedes doesn't return

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage decreased (-1.3%) to 95.679% when pulling 50dc140 on GavinDmello:master into be13dff on mcollina:master.

returnCode: 4
}, client.close.bind(client, done))
break
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about a check on err.returnCode if it's between 1 and 4, and use 4 instead? We can avoid the switch.

@mcollina
Copy link
Collaborator

mcollina commented Aug 2, 2016

Can you please add unit tests and docs?

@GavinDmello
Copy link
Collaborator Author

Cool. I'll make the changes :)

@GavinDmello
Copy link
Collaborator Author

Will there be test cases for each return code , or a generic one ? Also do I have to add documentation in the Readme ?

@mcollina
Copy link
Collaborator

mcollina commented Aug 2, 2016

I think you should have test cases for each return code, yes. You can set them up via a for loop.

Yes, documentation goes in the README.

Thanks!

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.02%) to 97.04% when pulling 6860e12 on GavinDmello:master into be13dff on mcollina:master.

returnCode: 4
}, client.close.bind(client, done))
return
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably avoid the first if.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was for checking if somebody doesn't send the returnCode property. I can merge it with the if below though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's what I meant :)

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.009%) to 97.027% when pulling 6e4bf48 on GavinDmello:master into be13dff on mcollina:master.

instance.authenticate = function (client, username, password, callback) {
callback({ returnCode: 1 }, null)
}
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please attach the returnCode to an Error object?

Copy link
Collaborator Author

@GavinDmello GavinDmello Aug 2, 2016

Choose a reason for hiding this comment

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

Do you mean something like this ?

var error = new Error("Auth error)
error.returnCode = 1
cb(error , null)

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.009%) to 97.027% when pulling 593c564 on GavinDmello:master into be13dff on mcollina:master.

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.009%) to 97.027% when pulling 2f81961 on GavinDmello:master into be13dff on mcollina:master.

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.009%) to 97.027% when pulling 657e2b4 on GavinDmello:master into be13dff on mcollina:master.

@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage increased (+0.009%) to 97.027% when pulling 75ffb68 on GavinDmello:master into be13dff on mcollina:master.

@mcollina mcollina merged commit 7798db8 into moscajs:master Aug 3, 2016
@mcollina
Copy link
Collaborator

mcollina commented Aug 3, 2016

That's perfect, thanks very much!

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.

None yet

3 participants