You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by velo <gi...@git.apache.org> on 2015/11/20 21:08:00 UTC
[GitHub] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
GitHub user velo opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/149
Fix fo TINKERPOP3-945
Added root cause to exceptions where a root cause is available
To make the decision of where to add root cause, I looked the impl (neo4j, hadoop, spark and orientDB) and add it when the root cause is available.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/velo/incubator-tinkerpop TINKERPOP3-945
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/149.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 #149
----
commit 72ec79f08f6dfabdc1593931296bf2a022977965
Author: Marvin Froeder <ve...@gmail.com>
Date: 2015-11-09T19:22:12Z
TINKERPOP3-945 - added root cause to exceptions where a root cause is available
----
---
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] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
Posted by velo <gi...@git.apache.org>.
Github user velo commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/149#issuecomment-160277255
FWIW, I rebased this so travis would pass
---
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] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
Posted by PommeVerte <gi...@git.apache.org>.
Github user PommeVerte commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/149#issuecomment-160421094
looks good to me.
VOTE: +1
---
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] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
Posted by velo <gi...@git.apache.org>.
Github user velo commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/149#issuecomment-159025666
I think is a nice idea, but that would have way bigger impact them what I did.
I can do it anyway, just say a word.
---
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] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/149#issuecomment-158979326
it's nice that you have "root cause" as optional to avoid breaking change. in that way, it is optional for providers to pass a cause and if they just want a vanilla exception they can use it that way. I guess that opens up the opportunity for there to be some inconsistency in the exceptions as one provider might choose to include cause and the other might not have a cause in the first place to throw....that might not matter i suppose.
I also wonder if all exceptions should have this option. It's a bigger change, but perhaps all exception methods should be deprecated and instead return some sort of `ExceptionBuilder<T extends Throwable>`. That way, the provider could do something like:
```text
throw Property.Exceptions.buildDataTypeOfPropertyValueNotSupported(value).cause(root).create();
```
or
```text
throw Property.Exceptions.buildDataTypeOfPropertyValueNotSupported(value).create();
```
as the need was required. Then this would be consistently applied. Of course, perhaps this specific issue is isolated as a problem and we aren't hiding "native" causes and messages elsewhere. If that's the case, then this pull request is the "simple" thing to do.
---
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] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/149#issuecomment-160644468
This change doesn't introduce breaks. @velo kept the old method signatures and provided a couple overloads. providers can choose to use the new methods or not.
---
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] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
Posted by okram <gi...@git.apache.org>.
Github user okram commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/149#issuecomment-160421174
VOTE +1. @spmallette -- will you handle the merge and any documentation on backwards compatibility? Also, if this is going to break for 3.1.1, then we should delay before merging when we get 3.2.0 set up.
---
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] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/149#issuecomment-159248203
I gave this some thought - your change solves a pretty specific problem so perhaps we should go with it for 3.1.1-incubating. The change I'd suggested is pretty big and probably should be discussed on the dev list before we dive into it. I do think it's a nice idea though but perhaps we reserve that for 3.2.0-incubating (which has not yet started development).
In the mean time we can add changes like the one you supplied as needed. Hopefully we won't need too many more before 3.2.0-incubating as it will just more methods/exceptions for us to deprecate later.
VOTE: +1
---
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] incubator-tinkerpop pull request: Fix fo TINKERPOP3-945
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/149
---
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.
---