You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by timse <gi...@git.apache.org> on 2015/02/16 09:11:26 UTC
[GitHub] thrift pull request: Trying to fix json protocol for nodejs
GitHub user timse opened a pull request:
https://github.com/apache/thrift/pull/379
Trying to fix json protocol for nodejs
this fixes: https://issues.apache.org/jira/browse/THRIFT-2994 (not sure if that is the correct way to link a jira ticket :smile: )
It initializes the arrays used to build up the json on construct and writes to transport whenever a `writeEnd` method is encountered and the stack lenth is 1, assuming its finished now.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/timse/thrift trying-to-fix-json_protocol-for-nodejs
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/379.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #379
----
commit 6855c207152f24d341f7783d1098bde3f275c0b8
Author: Tim Sebastian <ti...@gmail.com>
Date: 2015-02-13T17:18:23Z
initialize "tstack" and "tpos" in constructor
as all write methods require them to exist it does not seem to make too much sense to only make them available in the "writeMessageBegin" method
commit 2cd5e95b367c73981bff470cc7db1e04699871a6
Author: Tim Sebastian <ti...@gmail.com>
Date: 2015-02-13T17:23:56Z
introduce method "writeToTransport" to write to transport
- removed direct writing to transport from "writeMessageEnd"
- "writeToTransport" only acts if "tstack.length === 1", it assumed that we are finished then and the last item is the final json string
- call "writeToTranport" in the "flush" method
commit 6ce537a0ba93ff3af205360d804e41539b112e3b
Author: Tim Sebastian <ti...@gmail.com>
Date: 2015-02-13T17:26:13Z
call "writeToTransport" if we are the last item in the queue
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by timse <gi...@git.apache.org>.
Github user timse commented on a diff in the pull request:
https://github.com/apache/thrift/pull/379#discussion_r48103629
--- Diff: lib/nodejs/lib/thrift/json_protocol.js ---
@@ -89,9 +89,16 @@ TJSONProtocol.RType.set = Type.SET;
TJSONProtocol.Version = 1;
TJSONProtocol.prototype.flush = function() {
+ this.writeToTransportIfStackIfFlushable();
--- End diff --
oups! nice catch, sure. Will change. Thanks :)!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by timse <gi...@git.apache.org>.
Github user timse commented on the pull request:
https://github.com/apache/thrift/pull/379#issuecomment-166823413
anything else needed for this to proceed @nsuke ?
not sure what to do with the one failing test/if it has anything to do with these changes
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/379#discussion_r48103125
--- Diff: lib/nodejs/lib/thrift/json_protocol.js ---
@@ -89,9 +89,16 @@ TJSONProtocol.RType.set = Type.SET;
TJSONProtocol.Version = 1;
TJSONProtocol.prototype.flush = function() {
+ this.writeToTransportIfStackIfFlushable();
--- End diff --
Do you mean IfStack**Is**Flushable ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by timse <gi...@git.apache.org>.
Github user timse commented on the pull request:
https://github.com/apache/thrift/pull/379#issuecomment-145812777
i did changes, i don't really know what to do with the failing, tests. Are they related to my changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/379
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the pull request:
https://github.com/apache/thrift/pull/379#issuecomment-166881754
For that matter, even a single minimalistic test would be very useful to verify this and also to prevent future regression.
Either way, I think I can look into it soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by timse <gi...@git.apache.org>.
Github user timse commented on a diff in the pull request:
https://github.com/apache/thrift/pull/379#discussion_r48103666
--- Diff: lib/nodejs/lib/thrift/json_protocol.js ---
@@ -89,9 +89,16 @@ TJSONProtocol.RType.set = Type.SET;
TJSONProtocol.Version = 1;
TJSONProtocol.prototype.flush = function() {
+ this.writeToTransportIfStackIfFlushable();
--- End diff --
done :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by timse <gi...@git.apache.org>.
Github user timse commented on the pull request:
https://github.com/apache/thrift/pull/379#issuecomment-145458444
any news on that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-2994: Fix TJsonProtocol for nodejs
Posted by timse <gi...@git.apache.org>.
Github user timse commented on the pull request:
https://github.com/apache/thrift/pull/379#issuecomment-161125749
any news on this @RandyAbernethy
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---