Skip to content
This repository has been archived by the owner on Jan 17, 2020. It is now read-only.

Slightly improve invalid client cred error handling #177

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

Conversation

david-mcgillicuddy-moixa

It's certainly not perfect, but it changes a few of the panics into Errs. Unfortunately there's not much in the way of information that can be put into the errors since rusttls has an error type of ().

Closes #176

Copy link
Contributor

@TotalKrill TotalKrill left a comment

Choose a reason for hiding this comment

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

Basically fine, I consider this a bugfix however, even though it changes error output, but basically it just handles a crash, I would vote for merging and pushing to crates.io after changing the version to 0.31.1 instead.

Hopefully this library will reach a state where a v1.0.0 would be released. but until that time, and according to semver rules, we do not need to bump minor before a v1.0.0, even on api changes, and in this case the api change is minimal.

@@ -1,7 +1,7 @@
[package]
name = "rumqtt"
description = "Mqtt client for your IOT needs"
version = "0.31.0"
version = "0.32.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am personally split here, I would rather push for a 0.31.1 tag here instead of bumping minor, but it is a grey area since it actually changes return errors available.

But since the current jumps in this library has been major rewrites between minor releases, i still think doing this is better, It is actually a bugfix in essence from my point of view

Copy link
Author

Choose a reason for hiding this comment

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

It's a very minor (heh) bug fix yes but from a API semvar perspective, adding a new variant to a public enum not marked #[non_exhaustive] is a minor bump.

I personally would still do that even pre-1.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Your reasoning is sound and I do agree that pub enum changes are breaking api, but in this regard I feel you fix a crash and I would like to get changes out asap.

Because the other option is to not bump the version yet, since this change doesn't really warrant it.

Copy link

Choose a reason for hiding this comment

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

I just stumbled upon this panic. Would be good get better error message.

@david-mcgillicuddy-moixa
Copy link
Author

I'd forgotten all about this, what's the status? Seems like the 0.32.0 version is still valid and I don't see any bitrotting.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic with invalid key file
3 participants