Skip to content

Prepend table names to the generated enum names #716

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

Merged
merged 1 commit into from
Oct 23, 2020

Conversation

alecbz
Copy link
Contributor

@alecbz alecbz commented Sep 21, 2020

Otherwise we'll get a collision if a column name is shared for an enum column across two tables


This change is Reviewable

@@ -162,7 +163,7 @@ func (c *Catalog) createTable(stmt *ast.CreateTableStmt) error {
}
if col.Vals != nil {
typeName := ast.TypeName{
Name: col.Colname,
Name: fmt.Sprintf("%s_%s", stmt.Name.Name, col.Colname),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor annoyance with this is that it doesn't "singularize" the table's name.

It might also be frustrating to always opt-in to the longer enum names, when sometimes the terser names might suffice. But that seems trickier to fix

@alecbz alecbz force-pushed the alec/enum-prepend-name branch 2 times, most recently from 6818def to a57c517 Compare September 22, 2020 15:05
@kyleconroy
Copy link
Collaborator

I was going to say that this change is backwards incompatible, but this is the new MySQL implementation, so I'm okay making a breaking change. Not sure why you're test is failing, but I think this is overall a good change.

@alecbz
Copy link
Contributor Author

alecbz commented Oct 5, 2020

I was going to say that this change is backwards incompatible, but this is the new MySQL implementation, so I'm okay making a breaking change.

Oh, hm. Well I'm all for avoiding backwards incompatible changes, but I think in this case it's more or less unavoidable? Right now the generated code will cause name collisions on the generated "enum" Go types; not sure how else to resolve that.

That said, it might be worth spending more time deciding how the names should best be formed? E.g., if we want to get the de-pluralization to work, might be better to do it now than introduce another breaking change in the future?

@alecbz
Copy link
Contributor Author

alecbz commented Oct 19, 2020

@kyleconroy Any thoughts on ^ ?

If not, can fix the tests/go forward with this change.

@alecbz alecbz force-pushed the alec/enum-prepend-name branch 2 times, most recently from c817730 to 22607e5 Compare October 19, 2020 22:19
@kyleconroy
Copy link
Collaborator

kyleconroy commented Oct 20, 2020

I think this is fine as-is! Will merge once the tests have been updated and are passing.

Otherwise we'll get a collision if a column name is shared for an enum table across two tables
@alecbz alecbz force-pushed the alec/enum-prepend-name branch from 22607e5 to bb0cda5 Compare October 20, 2020 01:07
@alecbz
Copy link
Contributor Author

alecbz commented Oct 20, 2020

Sounds good -- tests green now

@kyleconroy kyleconroy merged commit e4607bb into sqlc-dev:master Oct 23, 2020
@kyleconroy
Copy link
Collaborator

Thank you!

@alecbz alecbz deleted the alec/enum-prepend-name branch October 23, 2020 17:43
@ghost
Copy link

ghost commented Oct 23, 2020

Thanks for this one. Drove me crazy with databases where many tables had type being an enum.
I was just overriding the type to a string in the config file and it felt wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants