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

Sequel ORM support #1277

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

lukesarnacki
Copy link

@lukesarnacki lukesarnacki commented Aug 12, 2019

I want to revisit the subject of sequel ORM support that was started in this issue: #1006

There is a PR that was started quite some time ago with an attempt to do so. I copied a lot of the code for fixtures setup and models definitions I cherry picked as many commits as I could from the original PR so kudos for @Aryk for starting this. I didn't want to start from that PR as some of the code is outdated and I wanted to start from database setup only (no core changes yet).

I want to start draft PR early for any suggestions that you may have and I want to discuss changes early to avoid pushing a lot of code. I am not sure what is the direction I want to go but I think I would like to change inheritance to composition and just delegate methods from Resource to object representing "active relation" or "sequel dataset" instead of inheriting from it. What do you think?

Aryk and others added 5 commits August 12, 2019 17:54
There was an attempt to write sequel adapter in cerebris#1030. I took commits
from this PR. This commit fixes most of the failures that are result of
applying these commits into much newer codebase. Most of the code is
also copied from the original PR.
@lukesarnacki
Copy link
Author

lukesarnacki commented Aug 13, 2019

I initially copied a lot of the code from the original PR but then I found a way to use original commits. There are still 4 failures on my local env and seems like CI has lots of them. So the first step would be to fix all of these failures for ORM=active_record and only then proceed with sequel.

Forgot to write in the PR description: please let me know if you are still willing to accept this PR. And when it comes to current work, are you ok with presented fixtures setup (in short: active record schema is dumped to a file with a rake task; it is then loaded into the database when test are run)?

@lgebhardt
Copy link
Member

@lukesarnacki I haven't taken a detailed look at this PR yet, but I am on board with adding the SequelRecordAccessor, though I wonder if it would be better as a separate gem. I'll try to dig into this soon and offer some guidance.

The CI failure should now be fixed if you rebase against master.

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.

3 participants