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

Bug: There seem to be many inconsistencies in the new sdk... #156

Open
2 tasks done
nickchomey opened this issue Oct 9, 2024 · 5 comments
Open
2 tasks done

Bug: There seem to be many inconsistencies in the new sdk... #156

nickchomey opened this issue Oct 9, 2024 · 5 comments
Labels
bug Something isn't working wip

Comments

@nickchomey
Copy link

Describe the bug

I'm having a lot of difficulty making sense of the new SDK because of a lot of inconsistencies between the various functions.

All of the functions have different type parameters (T, TResult, TWhat, etc...) and then function parameters and return types are inconsistent as well.

Some directly return the result of db.con.Send, while others pass it through an error check and return res.Result, nil

For example, Upsert() doesnt have any type parameters or return types at all!

Patch doesnt have Type parameters, while most do.

Merge and Relate use T any - others have TResult. No type for What though.

InsertRelation doesnt use any types at all

and much more.

Without any consistency across all of these things, it is very hard to get a grasp of what does what and what I should be doing.

Moreover, It makes it harder to implement anything as you have to consult each function to see what it needs, rather than having a standard format for all of them.

Steps to reproduce

na

Expected behaviour

Consistency

SurrealDB version

na

Contact Details

here

Is there an existing issue for this?

  • I have searched the existing issues

Code of Conduct

  • I agree to follow this project's Code of Conduct
@nickchomey nickchomey added the bug Something isn't working label Oct 9, 2024
@nickchomey
Copy link
Author

I'd go even further to say that a lot of the type stuff should probably just be hidden behind the api...

e.g. We should just be able to provide a string for table name, rather than use models.Table("tableName")

The more we have to use/mess with types etc, the more difficult it becomes to use

@remade
Copy link
Collaborator

remade commented Oct 11, 2024

@nickchomey

  1. T and TResult are used interchangeably for expected results. We can use TResult for result types. TWhat always refers to what is being queried or inserted (Table or Record ID)
  2. All expected non-nil result results have an error check. For functions that don't return a result, we are just interested if the request was successful
  3. You can use a string for table name.

@nickchomey
Copy link
Author

nickchomey commented Oct 19, 2024

Sorry, I didnt have a chance to look at this or your PR until now. I still see some issues

The various functions handle errors and responses quite differently
e.g. Create uses an if statement

var res connection.RPCResponse[TResult]
if err := db.con.Send(&res, "create", what, data); err != nil {
	return nil, err
}

return res.Result, nil

Patch doesn't have an if statement

var patchRes connection.RPCResponse[[]PatchData]
err := db.con.Send(&patchRes, "patch", what, patches, true)
return patchRes.Result, err

Delete and Upsert don't even return a response at all - just an error - thus no error check. Why? Therefore they also dont have a TResult Type Parameter.

Likewise Relate, InsertRelation and QueryRaw seem to be using pointers, so dont return a response. Why do they use pointers and nothing else does? They also dont have any Type Parameters, while the other functions do - neither does Patch.

Merge and Insert dont have a TWhat type parameter.

Perhaps I'm misunderstanding something, but it really would be much easier to use the sdk if there was complete consistency across all of the functions so that we dont have to remember or check the particulars of each.

They should all return a response, be consistent with usage of pointers, have the same type parameters, error checking, etc..

@remade
Copy link
Collaborator

remade commented Oct 31, 2024

@nickchomey Concerns raised are resolved in #163

@nickchomey
Copy link
Author

nickchomey commented Oct 31, 2024

It looks like many of the issues I raised weren't addressed, yet again... So, I'm repeating here most of what I wrote above...

  1. Patch still doesnt have any TResult, TWhat, etc... Type parameters. Shouldn't it?
  2. Insert still doesn't have a TWhat parameter.
  3. Likewise Relate, InsertRelation and QueryRaw seem to be using pointers, so dont return a response. Why do they use pointers and nothing else does? They also dont have any Type Parameters, while the other functions do - neither does Patch.

Perhaps I'm misunderstanding something, but it really would be much easier to use the sdk if there was complete consistency across all of the functions so that we dont have to remember or check the particulars of each.

They should all return a response, be consistent with usage of pointers, have the same type parameters, error checking, etc..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wip
Projects
None yet
Development

No branches or pull requests

2 participants