-
Notifications
You must be signed in to change notification settings - Fork 122
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
Structs tables #212
Structs tables #212
Conversation
Hey, thanks for your contribution, I'll get back to you in 2022. Happy new Year 💣 |
Kevin please review and test that. |
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.
Hey Pavle, sorry for the delay in reviewing this. Overall it looks pretty good, but added a few suggestions inline and there are a couple things need to be improved/fixed.
- User defined types containing a set cause the generation to fail with an error:
2022/04/22 08:55:35 failed to generate schema: render template: template: template: keyspace.tmpl:46:42: executing "keyspace.tmpl" at <$type>: wrong type for value; expected gocql.NativeType; got gocql.CollectionType
This was the specific type I saw it on:
CREATE TYPE backup_unit (
keyspace_name text,
tables set<text>,
);
- Internally ScyllaDB uses a hidden type called
empty
. When I tested this using thesystem
schema one of the tables has a column using theempty
type and it caused generation of code with a bogusEmptyType
type. I would add a special case to just skip columns with a type ofempty
entirely, since they are only used for internal purposes. - I would also add a few user types to your test output so that generation code is being tested as well.
cmd/schemagen/map_types.go
Outdated
return camelize(s) + "Type" | ||
} | ||
|
||
func getNativeTypeSting(t gocql.NativeType) 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.
func getNativeTypeSting(t gocql.NativeType) string { | |
func getNativeTypeString(t gocql.NativeType) string { |
cmd/schemagen/map_types.go
Outdated
return t | ||
} | ||
|
||
return camelize(s) + "Type" |
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.
Probably better to fatally log an error here instead of creating code that won't compile with a fake type name.
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.
Ah I misunderstood what this was doing since it made a fake type name of EmptyType
in my testing. I would perhaps just change the suffix on the user types instead to make this a bit more clear.
…dded test for udt.
Hey Kevin, thanks for feedback! I've fixed issue with collection type in udt, also I've added test for that. I've changed suffix for user types to "UserType", does this look good to you? |
Code generation for table structs