Feature/ios rest #4
Conversation
tiholic
requested review from
mattheworiordan,
paddybyers,
Ugbot and
QuintinWillison
and removed request for
mattheworiordan,
paddybyers and
Ugbot
There was a problem hiding this comment.
Nothing major in my comments - mainly food for thought.
It might be worth discussing next week bringing in checkstyle (or similar) for the Java code and perhaps switching on some of the stricter build and analysis checks for the Objective-C code... in both cases they can help spot simple issues at build, before they reach review.
There was a problem hiding this comment.
Thanks @tiholic.
I think my only feedback is:
- Saying you'll address a comment in a subsequent PR is fine if that PR is a new feature / capability. / spec item. However, if the subsequent PR fixes an issue in this PR, then the whole point of a review of code is to ensure we are happy with the code within this PR landing. Please have a think about how we can avoid this in future.
- I am struggling to comprehend what the interface is that our customers will be using with our Flutter SDK. Please can you ensure you update the README as you go along so that I can get a sense of this. From my perspective, the developer experience is absolutely the highest priority, so I want to see what that looks like.
- Do you think anything we are doing is not idiomatic for Dart/Flutter right now? I read https://blog.garrytan.com/masterclass-with-algolia-how-to-capture-the-heart-of-20m-software-developers the other day, and read about how Algolia preferred idiomatic code over consistency, whereas we prefer consistency where possible (but not always). I'd like to understand where you think we stand with this SDK in terms of idiomatic Dart vs Ably consistency in our APIs we present to our customers.
|
@mattheworiordan Coming to the idiomatic vs. consistency, as we are pretty consistent in terms on simple method calls. In context of event listeners, we will be having idiomatic approach using Streams. We are not inclined to following idiomatic approach w.r.t usage of named constructors. Standard constructors with asserts in initializer list is preferred over named constructors as that is most consistent with IDL. |
Sounds good.
Great, any reason not to do the latter then? |
Only reason is that the former is more consistent with IDL. @paddybyers @QuintinWillison and @Ugbot please share your inputs. We finalized this approach over the call. |
- Dart side will not pass the key if it does't have a value - Obj-C side will query incoming _dataMap for key which will return `nil` as key will not be available
|
@mattheworiordan had a long discussion with @QuintinWillison and we decided to follow idiomatic approach over consistency in case of named constructors. Code not following idiomatic approach will be updated in PRs raised here further. |
Hoping to solve this error in workflow environment: Unhandled exception: Invalid argument (uri): Unknown package: Instance of '_SimpleUri' #0 _resolveUri.<anonymous closure> (package:dartdoc/src/generator/resource_loader.dart:34:9) #1 _RootZone.runUnary (dart:async/zone.dart:1612:54) #2 _FutureListener.handleValue (dart:async/future_impl.dart:152:18) #3 Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:704:45) #4 Future._propagateToListeners (dart:async/future_impl.dart:733:32) #5 Future._completeWithValue (dart:async/future_impl.dart:539:5) #6 Future._asyncCompleteWithValue.<anonymous closure> (dart:async/future_impl.dart:577:7) #7 _microtaskLoop (dart:async/schedule_microtask.dart:40:21) #8 _startMicrotaskLoop (dart:async/schedule_microtask.dart:49:5) #9 _runPendingImmediateCallback (dart:isolate-patch/isolate_patch.dart:120:13) #10 _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:185:5) Error: Process completed with exit code 255.
Rest implementation for iOS part of the plugin.
Create a rest instance over ably instance and publish messages.