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-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-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-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 requests
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.js:338:15
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 fails
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 tests
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 comparison
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 available
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
14 matches
Mail list logo
