Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/519
---
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
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
Github user henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-107496515
Hi Igor,
Yes, we missed that too, I fixed another syntax error for the travis
version of node but I see now that your unit-test is actually failing (I
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 of
Github user henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-107734742
Thanks for pointing out Igor but it was broken before that change already
---
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-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 in
Github user henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-107748385
it doesn't pass in Travis CI (nor on my laptop), please setup the github
travis webhook on your fork so you can check it.
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 henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-107766703
http://docs.travis-ci.com/user/getting-started/#Step-one%3A-Sign-in
---
If your project is set up for it, you can reply to this email and have your
reply appear on
Github user henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-106700246
I should really had trusted Travis. This also broke my fork and I will have
to revert it unless someone provides a quick fix.
It seems to be breaking make check
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 asfgit closed the pull request at:
https://github.com/apache/thrift/pull/476
---
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
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
An
Github user henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-101781028
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 bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-101799446
What about Jasmine, Karma and PhantomJS as unit test suite?
and Protractor if we do e2e, what's best for nodejs? or node.js io.js and
javascript all in one?
Github user henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-101528449
1. e.g. lib/js/test/deep-constructor.test.js ? If you have the time it will
be perfect.
2. Yes, we have this duplication but at least it would be only loaded once,
Github user henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-101471617
Hi Igor,
Thanks for pinging and sorry for the delay here. I have been pretty busy
recently but can have a look at it later tonight.
Tests for the browser side
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 henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-99401213
Thanks for fixing it Igor but the acceptance tests make cross fail here:
https://travis-ci.org/apache/thrift/jobs/61195454#L5395
---
If your project is set up for
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 henrique commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-98653372
Hi Itkach,
I also had a look at your patch before and it looks valid but the problem
here is the BonkSet, which in C++ will require comparison operators, as you can
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 bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/476#issuecomment-98530788
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
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
28 matches
Mail list logo
