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

Avoid errors on duplicated create table calls #28

Closed
wants to merge 2 commits into from

Conversation

tetafro
Copy link

@tetafro tetafro commented Apr 14, 2022

It might be easier for clients if CreateTable method didn't return an error when the table already exists. This way it can be safely put in service init code, and called each time on start.

@ucirello what do you think?

@ucirello
Copy link
Collaborator

Thanks for this PR and sorry for taking long to get back to you. I wish you had opened an issue before doing so.

I think this PR is BAD. This is making an explicit error now implicit. Knowing that a table is there is important for the backward compatibility ethos typical of the Go ecosystem.

I believe that the proper handling moving forward would be to return a strong error type for when the table is already there. And then add modifier parameter (self referencing option pattern) to skip the table creation if already present.

Would you be willing to reshape this PR?

@ucirello
Copy link
Collaborator

In any case, reopen as an issue first. Thank you.

@ucirello ucirello closed this Apr 18, 2022
@tetafro
Copy link
Author

tetafro commented Apr 18, 2022

Sorry for not opening an issue, the code was short enough to just make a PR :)

I see your point, but I'm not sure about the details. For example, how can you say that it was a table creation error, if you can use any driver with UnsafeNew, therefore you can't get driver's typed error there?

Anyway, if you think that this feature is useful, it would be easier if you implement it the way you see it.

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

Successfully merging this pull request may close these issues.

2 participants