THRIFT-5712 - Added Dart 3 Compatibility #2930
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Known issues: - removed 2 tests from serializer_test.dart - t_framed_transport_test is not passing
Jens-G
requested changes
Feb 19, 2024
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.
| @@ -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 | |||
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?
Jens-G
reviewed
Feb 19, 2024
| @@ -19,14 +19,14 @@ name: tutorial_console_client | |||
| version: 0.20.0 | |||
| description: > | |||
| A Dart console client to implementation of the Apache Thrift tutorial | |||
| author: Apache Thrift Developers <dev@thrift.apache.org> | |||
| 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:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Known issues:
[skip ci]anywhere in the commit message to free up build resources.