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

Per-platform unique port identifiers #38

Merged
merged 1 commit into from
Sep 14, 2019
Merged

Per-platform unique port identifiers #38

merged 1 commit into from
Sep 14, 2019

Conversation

Boddlnagg
Copy link
Owner

@Boddlnagg Boddlnagg commented Mar 30, 2018

Implements #32.

This is not ready yet ... still missing:

  • Documentation
  • A way to get ports by value out of a MidiInputPorts or MidiOutputPorts collection (not possible via deref to slice)
  • CoreMIDI backend

@Boddlnagg Boddlnagg changed the title Port ids Per-platform unique port identifiers Mar 30, 2018
let mut handler_data = self.activate_callback();

// Create port ...
let port = match self.client.as_mut().unwrap().register_midi_port(port_name, PortIsOutput) {
let source_port = match self.client.as_mut().unwrap().register_midi_port(port_name, PortIsOutput) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of unwrap is disallowed in real life code. In this specific case, even when the function is returning a Result, there might be a panic, which would be breaking the contract of the function. I suggest you chain errors, or else use if let.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, that's usually true, but in this case, we know that self.client will actually never be None except between the self.client.take() in line 282 and the end of the method. And if it actually was None here, it's clearly a programming error on my side, and a panic is totally the right thing to happen then (it's a little bit like an implied assert!(self.client.is_some())).

It would be much better without using Option here, but I didn't find a way to do that when I originally wrote that code. I'll have to revisit this and see if there's a better way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, I've been quite involved in functional programming lately, and couldn't avoid to see that, but your argument is completely reasonable :-P
FYI While looking for possible alternatives I learnt about the do-notation in rust here, but I haven't used it myself yet.

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