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

queryValue requires awkward get.get!T #195

Open
schveiguy opened this issue Jul 15, 2019 · 8 comments
Open

queryValue requires awkward get.get!T #195

schveiguy opened this issue Jul 15, 2019 · 8 comments

Comments

@schveiguy
Copy link
Collaborator

schveiguy commented Jul 15, 2019

queryValue returns a Nullable!Variant. Let's say it returns a long (In my case, I was querying a COUNT(*) to get the total rows)

My code I put was:

auto total = conn.queryValue("select count(*) from ...").get!long

But this doesn't work, because Nullable intercepts the get call! So instead I have to do:

auto total = conn.queryValue("select count(*) from ...").get.get!long

Which IMO is ugly and annoying. Really this is an issue with Nullable and Variant reusing the same member name, but I doubt that will change.

Is it possible to instead specify the type to queryValue itself, and have it return a Nullable!T? something like:

// no need for get here, I know I'm going to get a value
long total = conn.queryValue!long("select count(*) from ...");
@schveiguy
Copy link
Collaborator Author

Or alternatively, you can use a different mechanism from Nullable to indicate an empty set.

@schveiguy
Copy link
Collaborator Author

I suppose this is moot since Phobos' Nullable will eventually require a get call.

However, I'll leave this open, because an updated API surely could do better than requiring get.get.

@schveiguy
Copy link
Collaborator Author

This would also be alleviated somewhat with the move to TaggedUnion as discussed in #175

@Abscissa
Copy link

Is it possible to instead specify the type to queryValue itself, and have it return a Nullable!T? something like:

Sounds like an excellent idea.

Only potential problem is that some of the queryValue overloads already take templated varargs (for prepared statements):

Nullable!Variant queryValue(T...)(conn, sql, T args) {...}
int myInt=...;
myConnection.queryRow("SELECT * FROM `myTable` WHERE `a` = ?", myInt)

It shouldn’t be a problem to change it to:

Nullable!T1 queryValue(T1, T2...)(conn, sql, T2 args) {...}

Buuuut….we’d probably want Variant (or rather, TaggedUnion, whatever...) as a default, and I’m wondering if that would be inherently ambiguous:

Nullable!T1 queryValue(T1=Variant, T2...)(conn, sql, T2 args) {...}

Assuming that works, we’d also need to decide how to handle the case when the type requested doesn’t match the type received. Attempt a silent conversion? Throw? Support both possibilities using different function names: queryValueA vs queryValueB (but with better names ;) )???

This would also be alleviated somewhat with the move to TaggedUnion as discussed in #175

Yea, TaggedUnion definitely sounds like a good idea. One of my other projects, SDLang-D I think, switched from Variant to TaggedAlgebraic some time ago (before TaggedUnion existed) and it definitely proved to be a good move.

@schveiguy
Copy link
Collaborator Author

Only potential problem is that some of the queryValue overloads already take templated varargs

That is solved with double-layer templates:

template queryValue(Ret = Variant)
{
    Nullable!Ret queryValue(Args...)(Connection conn, string sql, Args args) { ...}
}

Yes, still does full IFTI. At that point, you cannot specify the Args part of the template, but I'm assuming that is not a problem.

Assuming that works, we’d also need to decide how to handle the case when the type requested doesn’t match the type received

Existing behavior is to throw, so just throw.

@schveiguy
Copy link
Collaborator Author

schveiguy commented Oct 31, 2019

In regards to the switch to TaggedUnion, would probably be something like:

auto total = conn.queryValue("select count(*) from ...").get.longValue;

So really maybe this request will resolve itself once we get there.

@Abscissa
Copy link

That is solved with double-layer templates:
[...]
Existing behavior is to throw, so just throw.

Awesome, sounds great.

In regards to the switch to TaggedUnion, would probably be something like:

auto total = conn.queryValue("select count(*) from ...").get.longValue;

So really maybe this request will resolve itself once we get there.

Ahh, true. But maybe there's still value (perhaps for generic code?) in being able to specify the requested type as the actual type rather than via TaggedUnion's identifiers? In any case, the template idea is probably just a cleaner interface anyway: It's a little more idiomatic and doesn't require anyone to know anything whatsoever about TaggedUnion.

Oh, also, another thought on the whole migration thing: Maybe we could offer a simple util function to just convert the TaggedUnion to a Variant:

alias MySQLValue = TaggedUnion!blahblahwhatever...;

Variant toVariant(MySQLValue v) {...}
Nullable!Variant toVariant(Nullable!MySQLValue v) {...}
...
var = conn.queryValue(sql).toVariant;

Maybe that could even obviate the need for all those migration path gymnastics I outlined earlier (ugh, might've been in a different ticket...)

@schveiguy
Copy link
Collaborator Author

I suppose this is moot since Phobos' Nullable will eventually require a get call.

Don't know why I said this, it will still require get.get.

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

No branches or pull requests

2 participants