Add parameterized virtual properties #1755
Merged
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Introduction
This combines the best features of the previous two PRs. It's simple in its changes like #1474, but allows some control of which parameters go to which virtuals when calling
.toJSON()
like #1614.Also fixes the sinon warnings in the test suite and rearranges the Virtuals Plugin test file to nest some test cases under their respective describe blocks.
Motivation
No one else seemed interested in changing their PR.
Proposed solution
This builds on the previous work and works exactly like those two PRs when it comes to the changes done to the model's
.get()
method:However, it adds a new
virtualParams
option totoJSON()
that allows passing values to specific virtuals. It must be an object with keys corresponding with the virtual name their values are meant for:Closes #1474 and closes #1614.
Current PR Issues
Can't pass more than one argument to virtuals in
toJSON()
. This is possible when using.get()
directly though. This was done on purpose to allow passing arrays to virtuals, although the benefit of that is yet to be determined.Also lacks documentation, but that will be done separately on the Wiki page. In the future Plugin docs should be a part of the main website, but currently the docs for plugins aren't being generated.