search

agilebits / onepassword-app-extension

1Password Extension for iOS Apps
2.58k stars 311 forks source link

Assert the sourceView type for iPad. #358

Closed xingheng closed 7 years ago

xingheng commented 7 years ago

Hi, tab-style guys, this change add assertion for the sender/sourceView's type for iPad.

xingheng commented 7 years ago

Ping...

gks commented 7 years ago

@xingheng I'm not super fond of this change. I feel like asserting fast and early is going to be better than delaying the assert. Technically there is nothing wrong with the current code, so this is merely a code structure change up if I am understanding your change correctly.

gks commented 7 years ago

@xingheng Is there are particular scenario that the current existing code isn't properly handling for you? Perhaps some context there would be beneficial to understand why you feel this change needs to be made.

xingheng commented 7 years ago

@gks For my case the sender object could be any types, such as UITapGestureRecognizer from UILabel, it won't break me in debug mode in iPad and crash in Release mode, the original NSLog warning is too meek. I expect a strong assertion to find the crash issue ASAP, for iPad.

gks commented 7 years ago

Hi @xingheng

Unfortunately I'm going to have to close this PR. If you look at all of our assertions we've made the decision to assert at the very top. When designing an API we opted to do all the precondition checking at the top and assert immediately so the developer will know exactly where the error is.

In your particular change you've moved the assertion lower in the logic of the method. This means that the assertion is no longer doing precondition checking at the beginning and deferring it until later. Without doing an incredible amount of testing it's possible that a user might actually encounter an error due to this change but not be notified in the proper location. It would crash elsewhere in the UIActivityViewController rather than while creating the UIActivityViewController. This makes debugging the problem far more difficult.

As an alternative to this, my suggestion might be that if you really need this to work for you, perhaps fork the code, make your change and build using your own fork. You could always keep your fork in sync with ours and you'd have the change you need. But I don't want to put other developers at risk for this one and it's a fundamental change in code style than we are currently using.

If you would like to discuss further please email in at support+appex@agilebits.com and mention my name (Kyle) and we can discuss, but for now I am closing this PR.