-
Notifications
You must be signed in to change notification settings - Fork 330
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
Adding collectionKey
option to fix issue #31
#55
Conversation
If you set `collectionKey` to `true` on a HasMany relation (default), the relation's `key` attribute will be used to create a reference to the RelationalModel instance from the generated collection. Alternatively, if you set `collectionKey` to a string, it will use that string as the reference to the RelationalModel, rather than the relation's `key` attribute. If you set `collectionKey` to false (or any falsy value) this reference will not be created.
Looks good (special thanks for adding tests and documentation as well!). The only thing I'm concerned about would be overriding existing (including built-in) functions/properties on that collection. There are quite a few of these (including all the proxied underscore methods). I'm not sure if this should be an error, or just a logged warning (and wouldn't create the key in that case)? Making it an error would mean this change is not backwards compatible in cases where a relation is called So maybe something like the following would be better?
|
I actually already accounted for that case: check out commit e872613. |
I noticed, what I meant is that I'm not sure throwing an error is what we'd want here, or that a warning would suffice. Especially since an error can break existing code that doesn't use, and may not even want a |
Ah, I misinterpreted. A warning seems reasonable if it preserves backwards compatibility. Would you like to make that change and merge the pull request? |
Yes, that would be the best I guess. I could do it after merging, but then there would still be one commit with the |
If it's easier, I'll make the change in my fork, and then you can auto-merge. :) |
If that looks good, then you should be able to auto-merge. |
I needed to have a link from a child collection to a parent model, so I added an option to do so, and turned it on by default. I've also updated the README file and added tests. Please review and pull in!
Note that since this is adding a new, backwards-compatible feature to the project, the minor version of the project will need to be updated from the last release, as specified by Semantic Versioning