search

Rightpoint / RZBluetooth

Core Bluetooth helper library
Other
136 stars 47 forks source link

Fixes for issue #64 #65

Closed cpatterson-lilly closed 7 years ago

cpatterson-lilly commented 7 years ago
KingOfBrian commented 7 years ago

Hey @cpatterson-lilly -- This looks great. I'd really like to maintain compatibility on RZBSimulatedDevice with the callback methods though. Part of me wants to ditch compatibility because of the additional 6 methods it will require, but I think letting people upgrade with out breaking changes is more important.

cpatterson-lilly commented 7 years ago

@KingOfBrian Yeah, I understand the desire to avoid changing the API. Especially, in this case, which is pretty much an edge case.

I'll add back the original API calls that don't have a service UUID parameter, and those calls will search for the correct service UUID to use. If they find a single match (the usual case), all is well.

What should they do if they find multiple matches? Fail via assertion, throw an exception, or just return the first (possibly incorrect) match?

KingOfBrian commented 7 years ago

I think you can look up specifically (cUUID,sUUID) then generically (cUUID) you should be good. I'm not sure that I see a special error case from there.

On Fri, May 19, 2017 at 11:25 AM, Chris Patterson notifications@github.com wrote:

@KingOfBrian https://github.com/kingofbrian Yeah, I understand the desire to avoid changing the API. Especially, in this case, which is pretty much an edge case.

I'll add back the original API calls that don't have a service UUID parameter, and those calls will search for the correct service UUID to use. If they find a single match (the usual case), all is well.

What should they do if they find multiple matches? Fail via assertion, throw an exception, or just return the first (possibly incorrect) match?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Raizlabs/RZBluetooth/pull/65#issuecomment-302733546, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG-krVldQ58X7FqDYRbxSHSaaiAxVz2ks5r7bRugaJpZM4NegDh .

cpatterson-lilly commented 7 years ago

@KingOfBrian I'm asking about the case where you try to add a handler to an RZBSimulatedDevice by characteristic UUID alone (the original API), and it finds the same cUUID on multiple services -- what should the API do?

I'm leaning towards throwing the assertion failure; as a unit test developer I want to know about the problem sooner rather than later.

KingOfBrian commented 7 years ago

I was thinking that it wouldn't care, but I can see the argument for being a bit cautious about it. I'd be cool with adding an assert to protect that scenario. Sometimes I'll do exceptions over asserts to make sure they don't get disabled, but either would work for me.

On Fri, May 19, 2017 at 1:48 PM, Chris Patterson notifications@github.com wrote:

@KingOfBrian https://github.com/kingofbrian I'm asking about the case where you try to add a handler to an RZBSimulatedDevice by characteristic UUID alone, and it finds the same cUUID on multiple services -- what should the API do?

  • Throw an assertion failure (let the developer know about the conflict), or
  • Just add the handler for the first matching (cUUID, sUUID) it finds (which may cause tests to fail).

I'm leaning towards throwing the assertion failure; as a unit test developer I want to know about the problem sooner rather than later.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Raizlabs/RZBluetooth/pull/65#issuecomment-302768140, or mute the thread https://github.com/notifications/unsubscribe-auth/AAG-koFpthHGeJOJg8VqYr_teYZpigeqks5r7dXfgaJpZM4NegDh .

KingOfBrian commented 7 years ago

Ok, one last request @cpatterson-lilly -- can you revert the changes to the tests that aren't needed so both code paths are covered?

cpatterson-lilly commented 7 years ago

Will do.

cpatterson-lilly commented 7 years ago

@KingOfBrian Would you want to mark the original API methods deprecated, using __deprecated or __attribute__((deprecated)) or __attribute__((deprecated("message")))?

KingOfBrian commented 7 years ago

Looks great @cpatterson-lilly -- I owe you a 🍺. It won't be at this WW unfortunately, but someday!

cpatterson-lilly commented 7 years ago

Awwwww... someday for sure!