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.

Sign up for GitHub

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

Jump to bottom

Feature/platform message encoding #5

Merged
merged 6 commits into from
May 13, 2020
Merged

Feature/platform message encoding #5

merged 6 commits into from
May 13, 2020

Conversation

tiholic
Copy link
Contributor

@tiholic tiholic commented Apr 30, 2020

  • ErrorInfo encapsulation in PlatformException and AblyException
  • CodecPairs for better manageability
  • ObjectiveC code improvement

@tiholic tiholic requested a review from QuintinWillison April 30, 2020 07:59
mattheworiordan
mattheworiordan reviewed May 4, 2020
View reviewed changes
Copy link
Member

@mattheworiordan mattheworiordan left a comment

Choose a reason for hiding this comment

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

Thanks @tiholic

My thoughts:

  • This PR includes some of what I see in Feature/platform message encoding #5, which makes this review a) longer to do, b) harder to know what I am reviewing. Each PR should encapsulated sufficiently that it can be reviewed and landed on the branch it's based on, and should not include commits from other PRs (either it's self-standing and can be merged onto the feature branch, or it's a branch off another PR's branch).
  • I am still very concerned by the brittleness of the way value types are passed around, where we are not using a robust serializes data structure. This will inevitably break, and the bad part about this, is it could break really badly with just one innocuous change. I would like to see a solution to this problem, perhaps Protocol Buffers or something similar.

@tiholic tiholic changed the base branch from master to feature/ios_rest May 4, 2020 08:02
@tiholic
Copy link
Contributor Author

tiholic commented May 4, 2020

@mattheworiordan as @QuintinWillison suggested, I've updated target branches so reviews can be done better.

This was referenced May 4, 2020
Merged
Closed
@tiholic tiholic changed the base branch from feature/ios_rest to master May 5, 2020 16:51
@tiholic tiholic changed the base branch from master to feature/ios_rest May 5, 2020 16:53
Rohit R. Abbadi added 6 commits May 5, 2020 22:35
introducing CodecPairs and ErrorInfo decode
57aee00 
- for better managing encoding and decoding methods in custom Codec
- Adding decodeErrorInfo to get error info from platform channel
- retrieving ErrorInfo instance from platform exception to AblyException
Passing ErrorInfo from Android to Flutter
0a7eb83
Passing ErrorInfo from iOS to Flutter
d8c60f6 
- refactor codec related files into codec/
- AblyFlutterWriter added to pass objects from iOS to Flutter
- ErrorInfo from ably passed on to Flutter side via PlatformException
while publishing rest messages
Example updated with ErrorInfo details
f9737d8
toString implementation for ErrorInfo
c0c7723
simplify logging errorInfo in example
c11e35d
@tiholic tiholic force-pushed the feature/platform-message-encoding branch from 0c862fe to c11e35d Compare May 5, 2020 17:06
@tiholic tiholic changed the base branch from feature/ios_rest to master May 5, 2020 17:07
@tiholic
Copy link
Contributor Author

tiholic commented May 5, 2020

Rebased to resolve conflicts. Changed the target branch to master.

@QuintinWillison QuintinWillison mentioned this pull request May 7, 2020
Closed
This was referenced May 7, 2020
Closed
Closed
@tiholic tiholic merged commit 81e87bb into master May 13, 2020
@tiholic tiholic deleted the feature/platform-message-encoding branch May 13, 2020 18:12
tiholic pushed a commit that referenced this pull request May 20, 2020
custom Codegen for CodecTypes and PlatformMethods
20b5e63 
- mustache template based codegen
- generates codec types and platform methods
- addresses #5 (comment)
@tiholic tiholic mentioned this pull request May 23, 2020
Merged
tiholic pushed a commit that referenced this pull request Jun 20, 2020
@tiholic
custom Codegen for CodecTypes and PlatformMethods
97bd8ee 
- mustache template based codegen
- generates codec types and platform methods
- addresses #5 (comment)
tiholic pushed a commit that referenced this pull request Jul 6, 2020
@tiholic
custom Codegen for CodecTypes and PlatformMethods
dff379a 
- mustache template based codegen
- generates codec types and platform methods
- addresses #5 (comment)
@tiholic tiholic mentioned this pull request Aug 25, 2020
Merged
6 tasks
QuintinWillison added a commit that referenced this pull request Mar 19, 2021
@QuintinWillison
Add pub get step.
6cfcf59 
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.
@1N50MN14 1N50MN14 mentioned this pull request May 11, 2022
Closed
@sync-by-unito sync-by-unito bot mentioned this pull request Oct 26, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattheworiordan mattheworiordan mattheworiordan left review comments

@paddybyers paddybyers paddybyers left review comments

@QuintinWillison QuintinWillison QuintinWillison approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tiholic @mattheworiordan @paddybyers @QuintinWillison