-
Notifications
You must be signed in to change notification settings - Fork 58
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
Returning interface{}/any on most calls is not very flexible #18
Comments
If we want to use generics (locking us into go1.18+) we could have a cleaner solution something like this
and then the functions like |
This is a good option, but sadly as of go1.19 we can't have generic methods (only on the types over which the receiver is generic) so the function returning the Result would need to be top-level like surrealdb.Select[User](ctx, db, "* from user") and I find that less useful and organic than a simple Maybe this could exist as a replacement for the top-level Unmarshal ? |
My original branch I forked from #3 converted the Unmarshal (and RawUnmarshal that I added) to use generics so I could have syntax similar to the code in @Keitio's comment above, but I switched to I am currently working on the issue of multiple marshals / unmarshals mentioned in #17 and may be able to add a way to do this with |
If we want compatability then we should do interface{} i think. any is a go1.18 generic thing and is just an alias |
This isn't really a great implementation, but I got something that works: The functions now work like this:
I added an
Sorry, I meant |
for the Using another parameter to unmarshal into is nice, but locks us into non-generics sadly, and generics migration would be breaking or clunky |
So, since this ties into performance partially, I'm going to make a new issue and show an idea i had, which also ties into a way to avoid the interface{}/any responses and such, we can all discuss the performance/json unmarshalling side there |
@Keitio mentioned Mongo, so I tried to clean up my changes and mimic a bit of its driver code to see what it would look like. I ended up with something like this:
I added two structs that return from the
I actually like this a lot better than the previous attempts I've made because it inlines the unmarshaling. It also prevents use of the wrong unmarshal function (ie using the raw function for The branch with all my changes is here if anyone wants to browse through them or suggest changes.
Maybe this is because I don't have much experience in Go (mostly JVM background), but should a no result response really be an error? If I send a raw query using |
Basically for empty result, it depends. But either way, returning a bool and an error is bad practice, because it should be an error (on select 1) or a non-event (on select all). If you want to check for empty result, you can just directly do I'm all for this API though, I would just rename |
Yeah that's fair. I can switch the bool out for an error and make that signature change when I'm cleaning up the code later. |
My idea would be to return some sort of raw value that can be unmarshalled to anything (thus removing the need for the strange Unmarshal top-level function)
I don't know if this idea is good, as most libraries use a method param to unmarshal into, à la
The advantage of the first option would be that we can add other methods, for example
I can do the implementation of both, but am very open to any alternative as those do no strike me as very good
The text was updated successfully, but these errors were encountered: