thrift
thrift copied to clipboard
THRIFT-5712 - Added Dart 3 Compatibility
Known issues:
- removed 2 tests from serializer_test.dart
- t_framed_transport_test is not passing
- [x] Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [x] If your change does not involve any code, include
[skip ci]
anywhere in the commit message to free up build resources.
Hi Jens, Thanks for the feedback and for getting back to me so quickly. I will go through the comments, try to get TFramedProtocol working, and update the PR. Cheers! Paul
On Mon, 19 Feb 2024 at 22:53, Jens Geyer @.***> wrote:
@.**** requested changes on this pull request.
Thanks for the patch!
I have only a few rather simple comments/requests, but TBH I would really love to see TFramedProtocol working again. It is commonly used, either explicitly or sometimes also implicitly (some pieces of the server protocol stack may require framed at the client side).
If I can be of any help re general questions etc let me know.
In lib/dart/lib/src/console/t_web_socket.dart https://github.com/apache/thrift/pull/2930#discussion_r1495032364:
@@ -3,14 +3,14 @@ /// distributed with this work for additional information /// regarding copyright ownership. The ASF licenses this file /// to you under the Apache License, Version 2.0 (the -/// "License"); you may not use this file except in compliance +/// 'License'); you may not use this file except in compliance
Not sure why we need to modify the license header contents? There are more places I will not comment every single one.
In lib/dart/lib/src/transport/t_message_reader.dart https://github.com/apache/thrift/pull/2930#discussion_r1495033658:
@@ -1,3 +1,5 @@ +part of thrift;
Not sure why we need to modify the license header location?
— Reply to this email directly, view it on GitHub https://github.com/apache/thrift/pull/2930#pullrequestreview-1889193920, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFDPJ3TMJNWJPXCWKYR5H6DYUPCUXAVCNFSM6AAAAABDNJHS4SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOBZGE4TGOJSGA . You are receiving this because you authored the thread.Message ID: @.***>
Hi @baller-paul Any update on this?