You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by SimenB <gi...@git.apache.org> on 2016/09/02 11:21:06 UTC
[GitHub] thrift pull request #1075: THRIFT-3916 Throw proper errors from JS, not stri...
GitHub user SimenB opened a pull request:
https://github.com/apache/thrift/pull/1075
THRIFT-3916 Throw proper errors from JS, not strings
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/SimenB/thrift THRIFT-3916
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1075.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 #1075
----
commit 6800aed951cac7f9e519f601c0e5a99464f65b49
Author: Simen Bekkhus <sb...@gmail.com>
Date: 2016-09-02T11:19:56Z
THRIFT-3916 Throw proper errors from JS, not strings
----
---
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 issue #1075: THRIFT-3916 Throw proper errors from JS, not strings
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1075
@SimenB if you are interested in resurrecting this let me know - rebase on current master and push to CI will run all our tests.
---
[GitHub] thrift issue #1075: THRIFT-3916 Throw proper errors from JS, not strings
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1075
@SimenB if you are interested in resurrecting this let me know - rebase on current master and push to CI will run all our tests.
---
[GitHub] thrift issue #1075: THRIFT-3916 Throw proper errors from JS, not strings
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1075
@bananer is this worth pulling forward if it passes a build? Is it a breaking change?
---
[GitHub] thrift issue #1075: THRIFT-3916 Throw proper errors from JS, not strings
Posted by bananer <gi...@git.apache.org>.
Github user bananer commented on the issue:
https://github.com/apache/thrift/pull/1075
@jeking3 This is a well-intended change but I think it is not implemented correctly and will break error handling for users.
In some places in the compiler the input to `render_recv_throw` already is a proper `Error` (the JS exception class), e.g. https://github.com/apache/thrift/blob/2b09dfed9c6b858571e7d8829a2b4a4bcda18d6a/compiler/cpp/src/thrift/generate/t_js_generator.cc#L1648-L1652
This is not the case for other places:
https://github.com/apache/thrift/blob/2b09dfed9c6b858571e7d8829a2b4a4bcda18d6a/compiler/cpp/src/thrift/generate/t_js_generator.cc#L1673
The proper way of doing this would be to inspect all usages of `render_recv_throw` to see if the argument needs to be wrapped in `new Error(…)`. Still then, this could break error handling that relies on these cases being strings.
---
[GitHub] thrift issue #1075: THRIFT-3916 Throw proper errors from JS, not strings
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on the issue:
https://github.com/apache/thrift/pull/1075
@SimenB we have `Dokerfile` s for build env inside build/docker/ subdirectories.
You can also do `docker pull thrift/ubuntu` or thrift/debian.
---
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.
---