Through relations - Githubissues

inventid / tails

Models on the fly!

MIT License

1 stars 0 forks source link

Through relations #26

Closed joostverdoorn closed 10 years ago

joostverdoorn commented 10 years ago

Do not merge

This PR adds the through keyword to hasOne and hasMany relations. It allows to use or combine remote relations on the model. This feature is modeled after Rails' through functionality.

Aside from that, this PR adds a bunch of functionality that I want to run by you, @steffansluis @rogierslag, and best is to go through the specs that I added, they should be pretty self-explanatory.

I don't think this PR is finished yet, so please don't merge.

Edit: Things to be done:

[ ] Rewrite/rethink collection structure
[ ] Write tree traversal methods for relations and associations (optional)
[ ] Use the above tree traversal in through relations (optional)

coveralls commented 10 years ago

Coverage increased (+2.92%) when pulling 8358734e3f861a4fd39ac18f48951d44c107c20b on feature/through-relations into 9399bfb474c846c8c20d1f9797720a2a5aa91351 on develop.

steffansluis commented 10 years ago

There are a few things I want to mention. First of all, this is cool. Very cool! And it fits into the philosophy behind tails perfectly, so a very large virtual bucket of kudos for you @joostverdoorn! Secondly, we should think about how this will impact performance. Since these are pretty high level functions abstracted away behind an interface that is intended to be the center point of your model structure, this will probably impact loading times a bit. Maybe we should benchmark parts of the code at some point (when we actually have a release going or whatever).

rogierslag commented 10 years ago

I really like the concept, but I agree with Steffan. This part may be the core for many users of tails, so performance should also be a key issue here. Before we merge and release this, I'd like to run some internal benchmarks to check how it works with say 1.000 items, 10.000 items and 1.000.000. Stuff should scale linearly in the worst-case. If it goes any harder it might simply break down (from a users standpoint)

rogierslag commented 10 years ago

I think we should do this. Under 50 items it will hardly be able to cause any delays whatsoever and we can optimize later on. It would make tails so much better that it would be a waste not to include this!

joostverdoorn commented 10 years ago

Merging this for 0.1.0. This will probably receive major overhauls in the near future, but seems to be the most practical solution at this time.