-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: SQL module #148
feat: SQL module #148
Conversation
I've also done some refactoring to use |
21a1c24
to
50c88cd
Compare
modules/sql/db.go
Outdated
func (db *DB) Exec(ctx context.Context, args ...object.Object) object.Object { | ||
numArgs := len(args) | ||
if numArgs < 1 { | ||
return object.Errorf("type error: sql.query() requires at least one argument") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return object.Errorf("type error: sql.query() requires at least one argument") | |
return object.Errorf("type error: sql.exec() requires at least one argument") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thanks.
modules/sql/db.go
Outdated
} | ||
|
||
func (db *DB) Inspect() string { | ||
return "sql_conn()" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match conventions with types defined in other packages it looks like this should be sql.conn
. For example in exec the Command type string is exec.command
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thanks. I was following the pgx
module BTW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger, sounds like I need to update it to match.
modules/sql/db.go
Outdated
"github.com/risor-io/risor/op" | ||
) | ||
|
||
const DB_CONN object.Type = "db_conn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sql.conn
? See other comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. Just a couple minor suggestions on the type strings and attr names.
Signed-off-by: Luis Davim <dluis@vmware.com>
Thanks, I've updated the PR to address your comments. |
closes: #145