-
Notifications
You must be signed in to change notification settings - Fork 905
Add an emit_db_tags config option to support interoperability with gorp #656
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
Conversation
e8cff86
to
d490949
Compare
@@ -73,10 +73,17 @@ func buildStructs(r *compiler.Result, settings config.CombinedSettings) []Struct | |||
Comment: table.Comment, | |||
} | |||
for _, column := range table.Columns { | |||
tags := map[string]string{} |
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.
@kyleconroy This feels a little clunky -- happy to try and do something fancier/less repetitive.
Though given its only in four places (two of which are deprecated I think?), maybe not too bad in current form?
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.
I'm all for clunky and repetitive. The generation code is already a bit of a mess and I don't think this makes it any worse.
d490949
to
0b94957
Compare
This option is very similar to emit_json_tags, but it uses "db" as the tag name.
0b94957
to
ecb6d57
Compare
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.
Overall I'm really excited about this. It seems like a great interim solution for #364 and a way for people to slowly migrate queries from Go code to SQL.
GORM and sqlx also have tag support, so we should think about adding that in the future
@@ -113,6 +113,7 @@ type SQLGen struct { | |||
type SQLGo struct { | |||
EmitInterface bool `json:"emit_interface" yaml:"emit_interface"` | |||
EmitJSONTags bool `json:"emit_json_tags" yaml:"emit_json_tags"` | |||
EmitDBTags bool `json:"emit_db_tags" yaml:"emit_db_tags"` |
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.
sqlx also uses the db
tag for annotating structs. Can we change this value to emit_gorp_tags
? I want to make sure we have the flexibility to support other tools. This means we'd need to change the name of the test folder as well.
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.
sqlx uses the db
tag in the same way though, no? (I guess this depends on the resolution to that that other comment thread).
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.
I don't think sqlx has support for the autoincrement and primary key annotations? We'd need to confirm with both library authors that the annotations are compatible with each other
) | ||
|
||
type User struct { | ||
ID int32 `db:"id"` |
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.
According to the gorp docs, we could add a bit more information here
type User struct {
ID int64 `db:"id, primarykey, autoincrement"`
...
}
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.
-
Can you point me to this in the gorp docs?
-
Does this provide any value? AFAIK other attributes like that would only be used for creating tables based on gorp structs, but if we're using sqlc, the tables must have already been created.
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.
There doesn't appear to be any section in the docs. You can find a few different use cases in the quickstart.
type Post struct {
// db tag lets you specify the column name if it differs from the struct field
Id int64 `db:"post_id"`
Created int64
Title string `db:",size:50"` // Column size set to 50
Body string `db:"article_body,size:1024"` // Set both column name and size
}
// Example of using tags to alias fields to column names
// The 'db' value is the column name
//
// A hyphen will cause gorp to skip this field, similar to the
// Go json package.
//
// This is equivalent to using the ColMap methods:
//
// table := dbmap.AddTableWithName(Product{}, "product")
// table.ColMap("Id").Rename("product_id")
// table.ColMap("Price").Rename("unit_price")
// table.ColMap("IgnoreMe").SetTransient(true)
//
// You can optionally declare the field to be a primary key and/or autoincrement
//
type Product struct {
Id int64 `db:"product_id, primarykey, autoincrement"`
Price int64 `db:"unit_price"`
IgnoreMe string `db:"-"`
}
Also, there are apparently two versions of grop? v1 and v2?
Does this provide any value?
I think it does. If a field is marked as primary key,Insert
can return the ID of the created record.
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.
I think it does. If a field is marked as primary key, Insert can return the ID of the created record.
Ah yeah, that's true. Ok, I can add that then and rename the field to be gorp-specific.
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.
The other option, before you get started on the rename, is to have the emit_db_tags
only emit the column names. Then have a separate emit_gorp_tags
that does all the magic. My preference is for tool-specific configuration, because I doubt there are too many people using a combination of grop / sqlx / gorm.
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.
My preference would probably be to just leave it as the column names and not worry about the other gorp annotation stuff, since that's consistent across gorp/sqlx/gorm, and also gets us most of the value (all of the value for my specific use-case -- we use .SetKeys()
to explicitly specify the primary keys instead of the primarykey
tag).
But if we're going to include the extra gorp features now, yeah, I think having a single "gorp" option makes sense.
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.
My preference would probably be to just leave it as the column names and not worry about the other gorp annotation stuff
Great, let's go ahead and keep it as is, naming included. I'll open an issue tracking a future use case for orm-specific tags
This option is very similar to emit_json_tags, but it uses "db" as the tag name.
This allows us to use sqlc-generated models with gorp, which is especially useful as a workaround to #364.
This change is