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 2016/07/04 05:55:02 UTC
[GitHub] metamodel pull request #113: Dynamic DataContext factory (MM 1099)
GitHub user kaspersorensen opened a pull request:
https://github.com/apache/metamodel/pull/113
Dynamic DataContext factory (MM 1099)
This is my attempt at solving METAMODEL-1099. I've leveraged the standard [ServiceLoader](https://docs.oracle.com/javase/7/docs/api/java/util/ServiceLoader.html) for registering resource factories and data context factories. And I implemented this pattern with the various standard resources as well as the CSV data context. In my opinion this approach is pretty good, but I'd like to hear your feedback!
If you think this approach is good, we should obviously expand it to fit all the datacontext implementations. But that would be a rather trivial task I think, once we agree on this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kaspersorensen/metamodel METAMODEL-1099
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metamodel/pull/113.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 #113
----
commit 2173aa362a64015878d309a5b661bd5867563ee5
Author: Kasper S�rensen <i....@gmail.com>
Date: 2016-07-04T04:14:51Z
METAMODEL-1099: Added interfaces and registry for pluggable DC factory
commit 4b2e6a1f2e2daf1edeb4fd4c92d0daa873c697b9
Author: Kasper S�rensen <i....@gmail.com>
Date: 2016-07-04T04:21:35Z
METAMODEL-1099: Created a CsvDataContextFactory implementation
----
---
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 #113: Dynamic DataContext factory (MM 1099)
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metamodel/pull/113
---
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 issue #113: Dynamic DataContext factory (MM 1099)
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/113
Ok I'm gonna continue down this path 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 issue #113: Dynamic DataContext factory (MM 1099)
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/113
Overall idea is brilliant (of course I instantly think "Hey, OSGi ServiceFactory!", but this is close enough :smile:).
I'm _very_ happy nothing ended up in MetaModel-full as initially suggested in METAMODEL-1099, as that is something I think we should deprecate because of all the dependency issues it instantly brings to anyone using it. This might even make MM-full less interesting.
---
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 issue #113: Dynamic DataContext factory (MM 1099)
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/113
Added a JDBC factory too and fixed a bunch of the TODOs in the code. I suggest taking the PR as a candidate for merging now as it is quite a lot of work to make all the factories in one PR and I'd love to offer those as independent starter issues for people new to MetaModel.
---
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 #113: Dynamic DataContext factory (MM 1099)
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/113#discussion_r70035620
--- Diff: core/src/main/java/org/apache/metamodel/util/ResourceUtils.java ---
@@ -18,11 +18,31 @@
*/
package org.apache.metamodel.util;
+import java.net.URI;
+
+import org.apache.metamodel.factory.ResourceFactoryRegistryImpl;
+import org.apache.metamodel.factory.ResourceProperties;
+import org.apache.metamodel.factory.SimpleResourceProperties;
+import org.apache.metamodel.factory.UnsupportedResourcePropertiesException;
+
/**
* Static utility methods for handling {@link Resource}s.
*/
public class ResourceUtils {
+ public static Resource toResource(URI uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(String uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(ResourceProperties resourceProperties)
+ throws UnsupportedResourcePropertiesException {
--- End diff --
Joshua Block's _Effective Java_ item 62 is pretty clear:
> Use the JavaDoc `@throws` tag to document each unchecked exception that a method can throw, but do _not_ use the `throws` keyword to include unchecked exceptions in the method declaration
Of course that doesn't really solve the "how far down the chain should you go", but at least it doesn't look like a checked exception.
Regarding how far to document these, I think it's a good thing to do for stuff like the almost direct overloads here, and also methods closely coupled tightly enough that it would make sense.
---
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 #113: Dynamic DataContext factory (MM 1099)
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/113#discussion_r69839313
--- Diff: core/src/main/java/org/apache/metamodel/util/ResourceUtils.java ---
@@ -18,11 +18,31 @@
*/
package org.apache.metamodel.util;
+import java.net.URI;
+
+import org.apache.metamodel.factory.ResourceFactoryRegistryImpl;
+import org.apache.metamodel.factory.ResourceProperties;
+import org.apache.metamodel.factory.SimpleResourceProperties;
+import org.apache.metamodel.factory.UnsupportedResourcePropertiesException;
+
/**
* Static utility methods for handling {@link Resource}s.
*/
public class ResourceUtils {
+ public static Resource toResource(URI uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(String uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(ResourceProperties resourceProperties)
+ throws UnsupportedResourcePropertiesException {
--- End diff --
Ah right, but that's IMO more an issue with the overloaded methods :)
Maybe it's a bit of a religious discussion, but I would refer to some good (IMO) stackoverflow answers on the topic here:
http://stackoverflow.com/questions/20871763/should-i-declare-unchecked-exceptions-in-the-throws-specification
http://stackoverflow.com/questions/25743574/java-best-practice-for-declaring-unchecked-exception
---
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 issue #113: Dynamic DataContext factory (MM 1099)
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/113
Updated the ResourceUtils according to review feedback/discussion - added javadocs to explain why/when the exception can be expected.
---
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 issue #113: Dynamic DataContext factory (MM 1099)
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on the issue:
https://github.com/apache/metamodel/pull/113
`throws UnsupportedResourcePropertiesException` or not `throws UnsupportedResourcePropertiesException` seems more than a little bit chaotic. In some cases there are methods that calls a method which has `throws UnsupportedResourcePropertiesException` directly, without adding that to its own signature.
That is pretty confusing... In my mind, either `UnsupportedResourcePropertiesException` should be a checked exception (I know, I know... Will never happen), or not declared. This is just chaos.
---
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 #113: Dynamic DataContext factory (MM 1099)
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/113#discussion_r70132142
--- Diff: core/src/main/java/org/apache/metamodel/util/ResourceUtils.java ---
@@ -18,11 +18,31 @@
*/
package org.apache.metamodel.util;
+import java.net.URI;
+
+import org.apache.metamodel.factory.ResourceFactoryRegistryImpl;
+import org.apache.metamodel.factory.ResourceProperties;
+import org.apache.metamodel.factory.SimpleResourceProperties;
+import org.apache.metamodel.factory.UnsupportedResourcePropertiesException;
+
/**
* Static utility methods for handling {@link Resource}s.
*/
public class ResourceUtils {
+ public static Resource toResource(URI uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(String uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(ResourceProperties resourceProperties)
+ throws UnsupportedResourcePropertiesException {
--- End diff --
Hard to argue with consistency, no matter how bad that consistency is \u263a
---
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 #113: Dynamic DataContext factory (MM 1099)
Posted by LosD <gi...@git.apache.org>.
Github user LosD commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/113#discussion_r69680105
--- Diff: core/src/main/java/org/apache/metamodel/util/ResourceUtils.java ---
@@ -18,11 +18,31 @@
*/
package org.apache.metamodel.util;
+import java.net.URI;
+
+import org.apache.metamodel.factory.ResourceFactoryRegistryImpl;
+import org.apache.metamodel.factory.ResourceProperties;
+import org.apache.metamodel.factory.SimpleResourceProperties;
+import org.apache.metamodel.factory.UnsupportedResourcePropertiesException;
+
/**
* Static utility methods for handling {@link Resource}s.
*/
public class ResourceUtils {
+ public static Resource toResource(URI uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(String uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(ResourceProperties resourceProperties)
+ throws UnsupportedResourcePropertiesException {
--- End diff --
Here's one of the examples I talked about. Why do this declared the throwing, when the overloaded methods (just above) does not, even though they just call this (along with a constructor)?
---
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 issue #113: Dynamic DataContext factory (MM 1099)
Posted by tomaszguzialek <gi...@git.apache.org>.
Github user tomaszguzialek commented on the issue:
https://github.com/apache/metamodel/pull/113
Looks solid.
---
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 issue #113: Dynamic DataContext factory (MM 1099)
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on the issue:
https://github.com/apache/metamodel/pull/113
OK I will merge it in as per lazy consensus in terms of actual votes cast, and positive feedback remarks.
---
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 #113: Dynamic DataContext factory (MM 1099)
Posted by kaspersorensen <gi...@git.apache.org>.
Github user kaspersorensen commented on a diff in the pull request:
https://github.com/apache/metamodel/pull/113#discussion_r70124653
--- Diff: core/src/main/java/org/apache/metamodel/util/ResourceUtils.java ---
@@ -18,11 +18,31 @@
*/
package org.apache.metamodel.util;
+import java.net.URI;
+
+import org.apache.metamodel.factory.ResourceFactoryRegistryImpl;
+import org.apache.metamodel.factory.ResourceProperties;
+import org.apache.metamodel.factory.SimpleResourceProperties;
+import org.apache.metamodel.factory.UnsupportedResourcePropertiesException;
+
/**
* Static utility methods for handling {@link Resource}s.
*/
public class ResourceUtils {
+ public static Resource toResource(URI uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(String uri) {
+ return toResource(new SimpleResourceProperties(uri));
+ }
+
+ public static Resource toResource(ResourceProperties resourceProperties)
+ throws UnsupportedResourcePropertiesException {
--- End diff --
Ok so there are arguments on both sides here. But then I can throw another one: For consistency it makes sense to use the 'throws' keyword since that's what is being done in other core interfaces of MetaModel, including the most core of all: `DataContext` (has many `throws MetaModelException` declarations).
---
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.
---