Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sql2o should not be dependend on jodatime #38

Closed
aaberg opened this issue Jan 19, 2014 · 6 comments · Fixed by #47
Closed

Sql2o should not be dependend on jodatime #38

aaberg opened this issue Jan 19, 2014 · 6 comments · Fixed by #47
Assignees
Milestone

Comments

@aaberg
Copy link
Owner

aaberg commented Jan 19, 2014

instead, it should use jodatime if available.

@ghost ghost assigned aaberg Jan 19, 2014
@aaberg aaberg modified the milestones: 2.0, 1.3.0 Feb 3, 2014
@aldenquimby
Copy link
Contributor

I'm not too familiar with maven, but does this just mean adding <optional>true</optional> to the joda time dependency in the sql2o pom?

@aaberg
Copy link
Owner Author

aaberg commented Feb 8, 2014

Hmm. Could it be that easy?
I will try it right away. Test it a bit and see if it works.

@aldenquimby
Copy link
Contributor

So I just tried it out, and I think it works.

After reading this article, it might make sense to approach a lot of things like flyway does. I don't know if you are familiar with flyway, but it's a database migration library that has a similar situation with optional dependencies on many different database implementations.

Easiest way to handle it seems to be have optional dependencies, and check for existence at run time, which you are already doing with joda's DateTime in Convert.java. The article I linked above includes a useful utility for dealing with this:

public static boolean isPresent(String className) {
    try {
        Class.forName(className);
        return true;
    } catch (Throwable ex) {
        return false;
    }
}

...

if (isPresent("com.optionaldependency.DependencyClass")) {
    // This block will never execute when the dependency is not present
    executeCodeLinkingToDependency();
}

@aaberg
Copy link
Owner Author

aaberg commented Feb 8, 2014

Nice, this is definitely the way to go. I will assign you to this ticket, then I will try to do something similar with the slf4j dependency.

I think we should cache the result of the isPresent method after it has run, I suspect it may have a performance issue if not. Something like this:

private Boolean present = null;
public static boolean isPresent(String className) {
    if (present == null) {
        try {
            Class.forName(className);
            present = true;
        } catch (Throwable ex) {
            present = false;
        }
    }
    return present;
}

@aaberg aaberg assigned aldenquimby and unassigned aaberg Feb 8, 2014
@aldenquimby
Copy link
Contributor

Ok great. Looks like this strategy will work for dealing with Convert.java, but it will not work with addParameter(String name, DateTime value) in Query.java. I will look into possible solutions and send you a pull request for review when I'm done

@aaberg
Copy link
Owner Author

aaberg commented Feb 9, 2014

I have a suggestion:
I think it will work to just remove the addParameter(String, DateTime) method, and detect and handle jodatime parameters within the addParameter(String, Object) method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants