Fix compound documents includes #1
Conversation
| ], | ||
| } | ||
| includes = ['long-comments', 'long-comments.user'] | ||
| includes = ['long-comments.user'] |
There was a problem hiding this comment.
Now you're testing only once include, not an array as before, and you did that everywhere.
From the specs it looks like you're changing functionality instead of extending, but that's not what you're doing.
There was a problem hiding this comment.
Exactly. Im changing it cuz it was incorrectly implemented with the standard.
Take a look here: fotinakis#116
Right now including compound relations will also include all chain linkage.
Specs are in 100% correct :)
There was a problem hiding this comment.
What if I do includes = ['long-comments.foo', 'long-comments.user']?
Or includes = ['upvotes', 'long-comments.user']?
There was a problem hiding this comment.
lol, all I'm saying is that the previous spec had multiple elements in the includes array and now it only has one. I see that you'd have to add new specs for that or it'd change the assert, but are you sure that those will work as intended? Wouldn't the first example I included return long-comment twice?
There was a problem hiding this comment.
That's a valid concern, but I think it would be best to get back to it once we have Inbox ready, this is already merged anyway and we are running out of time really. I think there are a couple of more things to be fixed in this gem 😅
There was a problem hiding this comment.
this gem is missing a lot of tests :D
There was a problem hiding this comment.
Yes, that's not cool. but I think we will migrate from it sooner or later
There was a problem hiding this comment.
One beer says it's later! 😄 It gets the job done :P
There was a problem hiding this comment.
I know :D But Im not deciding when 😅
|
Whats the problem here? Works fine |
3 participants
No description provided.