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

contrib: add gorm.io/gorm #759

Merged
merged 27 commits into from
Jan 4, 2021
Merged

contrib: add gorm.io/gorm #759

merged 27 commits into from
Jan 4, 2021

Conversation

AdallomRoy
Copy link
Contributor

@AdallomRoy AdallomRoy commented Oct 9, 2020

Fixes #684

@AdallomRoy AdallomRoy marked this pull request as draft October 9, 2020 13:19
@gbbr gbbr changed the title Add gormv2 contrib: add gorm.v2 Oct 9, 2020
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

I recommend taking a look at other integrations to figure out what the style conventions and recommendations are for naming etc.

Once it's ready for review, just take it out of draft.

contrib/gorm.io/example_test.go Outdated Show resolved Hide resolved
contrib/gorm.io/gorm.go Outdated Show resolved Hide resolved
@gbbr gbbr changed the title contrib: add gorm.v2 contrib: add gorm.io/gorm Oct 9, 2020
@AdallomRoy
Copy link
Contributor Author

Thank you for your comments. I will first make sure all tests pass and then re-order everything to be according to your coding conventions

@AdallomRoy AdallomRoy requested a review from gbbr October 9, 2020 16:16
@AdallomRoy AdallomRoy marked this pull request as ready for review October 9, 2020 16:16
@AdallomRoy
Copy link
Contributor Author

I'm pretty sure this is in accordance to your guidelines.
The tests that fail seem unrelated to my work, I'm not sure how stable the tests are normally, but please let me know if you think otherwise.

gbbr
gbbr previously requested changes Oct 12, 2020
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Thanks for doing the work. You'll need to bear with us for a few more rounds of reviews but I think we're on the right path.

As for the CI test, no worries, we will look at that separately. It does seem unrelated to your PR. However, you will have to use gofmt on your code.

contrib/gorm.io/gorm/gorm.go Outdated Show resolved Hide resolved
contrib/gorm.io/gorm/gorm.go Outdated Show resolved Hide resolved
contrib/gorm.io/gorm/gorm.go Outdated Show resolved Hide resolved
contrib/gorm.io/gorm/gorm.go Outdated Show resolved Hide resolved
contrib/gorm.io/gorm/gorm.go Outdated Show resolved Hide resolved
contrib/gorm.io/gorm/example_test.go Outdated Show resolved Hide resolved
contrib/gorm.io/gorm/example_test.go Outdated Show resolved Hide resolved
@gbbr
Copy link
Contributor

gbbr commented Oct 15, 2020

CI is fixed. If you still plan on continuing the PR, once you rebase on v1 branch it should be fine (for the ci/circleci: test-contrib part)

@AdallomRoy
Copy link
Contributor Author

Thanks!

@knusbaum knusbaum added this to the 1.28.0 milestone Oct 20, 2020
@knusbaum
Copy link
Contributor

Sorry, I've only now had a chance to take a look at this. Previously (in #714) I had looked into making a Plugin to add our tracing, as that looks like a simpler interface for the user. They would simply be able to write:

db.Use(gormtrace.Plugin())

@AdallomRoy What do you think about a Plugin? Do we lose anything by not using the Dialector interface?

@AdallomRoy
Copy link
Contributor Author

Nice! I can check this out later this week and let you know

This was referenced Mar 13, 2021
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.

contrib/jinzhu/gorm: support v2
6 participants