THRIFT-5712 - Added Dart 3 Compatibility #2930
Conversation
Known issues: - removed 2 tests from serializer_test.dart - t_framed_transport_test is not passing
There was a problem hiding this comment.
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.
| /// 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 |
There was a problem hiding this comment.
Not sure why we need to modify the license header contents?
There are more places I will not comment every single one.
| @@ -1,3 +1,5 @@ | |||
| part of thrift; | |||
|
|
|||
There was a problem hiding this comment.
Not sure why we need to modify the license header location?
| description: > | ||
| A Dart console client to implementation of the Apache Thrift tutorial | ||
| author: Apache Thrift Developers <[email protected]> | ||
| homepage: http://thrift.apache.org |
|
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
<#2930 (comment)>:
> @@ -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
<#2930 (comment)>:
> @@ -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
<#2930 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BFDPJ3TMJNWJPXCWKYR5H6DYUPCUXAVCNFSM6AAAAABDNJHS4SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOBZGE4TGOJSGA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Hi, any news on the dart3 support. Since android playstore is enforcing 16K alignment and for apps build with flutter, that means an update to dart3 otherwise no 16 alignment for the libraries. Best Regards, |
| // read another frame of data (taken from the Java implementation) | ||
| // NOT taken from the dart 1 implementation | ||
| // NOT SURE IF THIS IS THE RIGHT WAY TO DO IT | ||
| await _readFrame(); |
There was a problem hiding this comment.
this would change functionality , wouldn't it?
@baller-paul seems no longer working on this? Anyway, if the remaining issues get resolved, I would be happy to help bringing this forward. |
4 participants
Known issues:
[skip ci]anywhere in the commit message to free up build resources.