-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
Associations (one-to-many, many-to-one) #244
Conversation
Added Associations API docs
# entity Article | ||
# attribute :id, Integer | ||
# attribute :user_id, Integer | ||
# association :user, [User], foreign_key: :id, collection: :articles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this example should be User
instead [User]
because we have a singular association
@AlfonsoUceda no problems. Thanks for review'ng. I'm gonna take a look at it and create the missing test as well. |
# query.preload(:articles) | ||
# end | ||
# end | ||
def preload(association) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think to add splat operator? I think is better this api
preload(:users, :articles)
than preload(:users).preload(:articles)
def preload(*associations)
associations.each do |association|
@associations << association
end
end
cc @lotus/core-team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I prefer your version, but I'd rather don't break the Query's where
api contract i.e query.where(foo: :baz).where(baz: :qux)
. If it isn't a "problem", then I can change, no problems!
In commit, will allow us to build internal queries aligned and preload their associations as well. i.e. def self.by_category(category) query.where(category_id: category.id).preload(:category) end
Hey @hlegius nice work! @AlfonsoUceda is there anything impeding this PR to be merged? Anything I can do? I'm currently implementing an example of Association with Lotus and it would be awesome to have this feature. |
@hlegius This is a great work! 👏 What prevents us to merge this is the lack of implementation of the other half of the job: writing associated entities. Please have a look at this gist: https://gist.github.com/jodosha/431091eebb41606b358a /cc @lotus/core-team @andresilveira |
@jodosha is there anything that I can do to help this feature get merged? I mean, what's your plans to do before merge it? maybe I could help o/ /cc @AlfonsoUceda |
Looking forward to this!!! 👍 |
@hlegius Mr @joneslee85 (PING) will help you to finalize this. 😸 |
@hlegius polite ping from me, grab me and we work this out together |
@jodosha do you think it still will be useful somehow? If yes, @joneslee85 can I ping you on Gitter to talk about schedule/tasks to do? |
@hlegius @joneslee85 Please hold on until we'll release 0.6 |
@collection.associations[:user].must_be_instance_of Lotus::Model::Associations::ManyToOne | ||
end | ||
|
||
it 'defines a many to one association' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think description should be 'defines a one to many association'
/cc @Heruku
I've copied #56 from heruku and started to grow the association
.preload
idea (started by him) to get it working properly.First, I tried to fork heruku's PR and rebase from there, but I've got no success on it due tons of new features which has been added since them. So, I decided to fork Lotus' repo, get diff from #56 and reapply file by file, paying attention to each new detail.
Todo:
ArticlesRepository.with_category.published.all
ArticlesRepository.published.with_category.all
Known Limitations (so far)
Also, refers to #35