Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/519#issuecomment-114331068
Any objections to merging 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
GitHub user itkach opened a pull request:
https://github.com/apache/thrift/pull/519
THRIFT-3122 Convert plain js objects given as thrift struct constructor
arguments to thrift structs
You can merge this pull request into a Git repository by running:
$ git pull https
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-110092916
I enabled travis hook for my fork and pushed test fix
(https://github.com/itkach/thrift/commit/861d262c66f21ad85343ef419d0081ba294e158a)
to trigger the build (https
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-107763291
> setup the github travis webhook on your fork
Are there instructions on how to do it? Looks like builds are triggered
automatically only on open pull reque
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-107744119
> Thanks for pointing out Igor but it was broken before that change already
How? What was the error? All tests in
`nodejs/test/deep-constructor.test.js` pass
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-107519120
> I fixed another syntax error for the travis version of node but I see now
that your unit-test is actually failing
It appears that test is failing because
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-106797978
Relevant error is
```
Error: Cannot find module './gen-nodejs/JsDeepConstructorTest_types'
at Function.Module._resolveFilename (module
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-105526647
Copy functions are now in libs, for js and node, tests for js are added.
While porting tests to use json protocol for js/browser I noticed a fix for
list values in maps
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-101651241
> e.g. lib/js/test/deep-constructor.test.js ?
So more duplication, except it also needs to be rewritten for the different
test runner js uses
&
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-101486364
> Tests for the browser side would be awesome
Existing browser tests pass when executed locally (make check with Java
server is successful and grunt test fa
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-101330756
Are there any other concerns I need to address to have this change merged?
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-99608147
I see that the build failed again:
https://travis-ci.org/apache/thrift/jobs/61506900#L5435
but it doesn't look like this is related to my changes. When I run
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-98714766
> BonkSet, which in C++ will require comparison operators,
That is a strange requirement and it's not clear how would one go about
providing such co
Github user itkach commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-98543767
> nice idea, but the introduction of new structs within
test/ThriftTest.thrift breaks many other language tests, do you really need new
structs? e.g. BonkMap is availa
GitHub user itkach opened a pull request:
https://github.com/apache/thrift/pull/476
THRIFT-3122 Convert plain js objects given as thrift struct constructor
arguments to thrift structs
You can merge this pull request into a Git repository by running:
$ git pull https
15 matches
Mail list logo
