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

add bulk upsert function #28

Closed
wants to merge 1 commit into from
Closed

Conversation

Betula-L
Copy link

@Betula-L Betula-L commented Apr 1, 2020

No description provided.

@bombsimon
Copy link
Collaborator

This looks like an awful lot of duplication. I think it would be better to create one method that takes an bulk action (INSERT, INSERT IGNORE, INSERT ... ON DUPLICATE KEY etc) and then just pass that query to the bulk function. By doing this there will be a lot of places to keep track of and update when needed. You can have a look at this implementation as a reference.

Sorry for shameless self promotion, although not saying you should switch library, just linking as an example.

Copy link
Collaborator

@bombsimon bombsimon left a 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 this is a good way forward for this library, I would rather see keeping the signature but restructuring the code in a way that each query type uses the same code to generate columns and values.

@bombsimon
Copy link
Collaborator

Also, don't spend too much time on this, according to https://github.com/jinzhu/gorm/issues/255#issuecomment-590287329 this bulk feature will be featured in Gorm v2 which will be released soon.

@Betula-L
Copy link
Author

Betula-L commented Apr 7, 2020

OK. Thanks.

@Betula-L Betula-L closed this Apr 7, 2020
@bombsimon
Copy link
Collaborator

@Betula-L Didn't mean you should close this! The hint about the coming feature was just so you don't feel you spend unnecessary time but I'll gladly accept the feature in this project!

I think the feature you added is great, I just thing a different approach would be better to improve maintainability and keep it more DRY.

Please feel free to reopen this issue and I'll gladly help you out with tips and/or ideas!

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.

None yet

2 participants