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

Add LibUsbError and InitFalied error variant. new returns Result. Bum… #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cr1901
Copy link
Contributor

@cr1901 cr1901 commented Apr 25, 2020

…p version.

Rationale:

  • As opposed to a string, LibUsbError has private fields which can change without breaking semver.
  • Fallible construction is a reasonable API design. c.f. File::open. Add a variant to deal with that particular failure.
  • Since all libftdi1 strings are ASCII, converting to UTF8 genuinely won't fail and I'd prefer not to panic in a library (though unreachable! already prevents that).
  • bindgen feature is exposed- disabled by default.

@tanriol
Copy link
Owner

tanriol commented Apr 25, 2020

Bundling all of this in one commit is not a good idea :-)
Also, I don't like the idea of every builder step being fallible. If you insist on handling the context initialization failure, I'd prefer to defer creating the actual context until the final step.

@cr1901
Copy link
Contributor Author

cr1901 commented Apr 25, 2020

@tanriol I will fix after the weekend; I need a break, otherwise I risk burning out with the other things I'm working on.

@tanriol
Copy link
Owner

tanriol commented Apr 25, 2020

Sure, no hurry.

@cr1901
Copy link
Contributor Author

cr1901 commented May 6, 2020

@tanriol I'm in a much better headspace now after a break. I'll work on incorporating your suggestions into the non-draft PRs later today.

Also, I sent an email discussing some further developments; I think I should keep you in the loop.

@cr1901 cr1901 mentioned this pull request May 8, 2020
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.

2 participants