You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@metamodel.apache.org by kaspersorensen <gi...@git.apache.org> on 2015/08/25 10:53:17 UTC
[GitHub] metamodel pull request: Converted OperatorType from an enum to an ...
GitHub user kaspersorensen opened a pull request:
https://github.com/apache/metamodel/pull/47
Converted OperatorType from an enum to an interface
This branch is not yet functional, _but_ I wanted to raise the idea of making OperatorType an interface instead of an enum. We recently did the same with FunctionType and it enables us to potentially plug in new implementations without breaking backwards compatibiltity. In the case of OperatorType I imagine that some custome operators could be plugged in to support custom cases.
I didn't quite finish this PR/branch yet because I ran into problems. The main one being deserialization of old OperatorType enum objects. I think this can be overcome with a hack a la the thing we have with LegacyColumnType.
Another issue is to get rid of any remaining switch-case blocks which rely on OperatorType to be an enum.
But before handling those issues I wanted to take the temperature in the community - is this good/worth doing?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kaspersorensen/metamodel feature/operator-as-interface
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/47.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 #47
----
commit 052fa28586efe39c833a90d973505949dd504594
Author: Kasper Sørensen <i....@gmail.com>
Date: 2015-08-25T08:47:10Z
Converted OperatorType from an enum to an interface
----
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by ardlema <gi...@git.apache.org>.
Github user ardlema commented on the pull request:
https://github.com/apache/metamodel/pull/47#issuecomment-135532706
Absolutely! I think this is worth doing.
As we've already seen with other cases transforming some enums to interfaces will open up a wider range of possibilities and this case is a clear example. So please go ahead with this PR.
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on the pull request:
https://github.com/apache/metamodel/pull/47#issuecomment-139462982
LGTM
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/47
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/47#issuecomment-139566423
OK I'll merge it then.
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by albertostratio <gi...@git.apache.org>.
Github user albertostratio commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/47#discussion_r39131511
--- Diff: core/src/test/java/org/apache/metamodel/query/OperatorTypeTest.java ---
@@ -23,12 +23,12 @@
public class OperatorTypeTest extends TestCase {
public void testConvertOperatorType() throws Exception {
- assertEquals(OperatorType.EQUALS_TO, OperatorType.convertOperatorType("="));
- assertEquals(OperatorType.GREATER_THAN, OperatorType.convertOperatorType(">"));
- assertEquals(OperatorType.LESS_THAN, OperatorType.convertOperatorType("<"));
- assertEquals(OperatorType.DIFFERENT_FROM, OperatorType.convertOperatorType("<>"));
- assertEquals(OperatorType.LIKE, OperatorType.convertOperatorType("LIKE"));
- assertEquals(OperatorType.IN, OperatorType.convertOperatorType("IN"));
- assertEquals(null, OperatorType.convertOperatorType("foo"));
+ assertEquals(OperatorType.EQUALS_TO, OperatorTypeImpl.convertOperatorType("="));
+ assertEquals(OperatorType.GREATER_THAN, OperatorTypeImpl.convertOperatorType(">"));
+ assertEquals(OperatorType.LESS_THAN, OperatorTypeImpl.convertOperatorType("<"));
+ assertEquals(OperatorType.DIFFERENT_FROM, OperatorTypeImpl.convertOperatorType("<>"));
+ assertEquals(OperatorType.LIKE, OperatorTypeImpl.convertOperatorType("LIKE"));
+ assertEquals(OperatorType.IN, OperatorTypeImpl.convertOperatorType("IN"));
+ assertEquals(null, OperatorTypeImpl.convertOperatorType("foo"));
}
--- End diff --
Should we add the other "cases"?? ("==", "EQ", "EQUALS_TO"...)
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/47#issuecomment-139023199
OK I did an update of the branch. Will also create a JIRA issue now. Please review :-)
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/47#issuecomment-139345262
Addressed the unittest request from @ardlema
Regarding your remark @LosD - yeah, I think this is unavoidable. And at least it is consistent with how the ColumnType and FunctionType interfaces are implemented.
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the pull request:
https://github.com/apache/metamodel/pull/47#issuecomment-139169283
LGTM, except that I don't really like that OperatorType is dependent on OperatorTypeImpl. It's probably unavoidable if you want to keep it compatible, but it tastes... Dirty.
---
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] metamodel pull request: Converted OperatorType from an enum to an ...
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the pull request:
https://github.com/apache/metamodel/pull/47#issuecomment-139024042
This is my suggested fix for METAMODEL-188
---
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.
---