Feature/add reducer alias for transform entities #209
Conversation
… for transform alias ViacomInc#167
… test for checking if reducer i ViacomInc#167
…entation for transform entitiy ViacomInc#167
packages/data-point/lib/core/core.js
Outdated
| @@ -92,6 +93,7 @@ function create (spec) { | |||
| // supports currying | |||
| manager.resolve = Transform.resolve(manager) | |||
| // does not support currying | |||
| manager.reducer = _.partial(Transform.transform, manager) | |||
There was a problem hiding this comment.
we don't need this line, because it's adding a new function to the api, which is not part of issue #209
There was a problem hiding this comment.
One of the unit tests in core.test.js fails without this line of code
There was a problem hiding this comment.
All the tests on master are currently passing. If you're referring to the test that you added specifically for this line, that test should also be removed when this line is deleted.
There was a problem hiding this comment.
Left a few small comments, but would like if this PR added more tests to ensure that reducer entities work properly. Since it'll effectively duplicate the coverage maybe we don't need much, but we should at least check some of the lifecycle methods are working on this entity type.
If there is a plan to deprecate transform entities in favor of reducer, everything will have to be tested so we don't lose code coverage. That can probably wait though.
| @@ -82,6 +82,9 @@ | |||
| "avatar_url": "https://avatars2.githubusercontent.com/u/16357592?v=4", | |||
| "profile": "https://github.com/victorsingh", | |||
| "contributions": [ | |||
| "code", | |||
| "doc", | |||
| "test", | |||
packages/data-point/README.md
Outdated
| @@ -1510,6 +1510,42 @@ dataPoint.addEntities({ | |||
| ``` | |||
| </details> | |||
|
|
|||
| Note: We recommend using reducer as an alias for transform | |||
There was a problem hiding this comment.
Can we add backticks around the names?
We recommend using
reduceras an alias fortransform.
Also, is there a plan to deprecate the transform entity type entirely? If that's true it should be mentioned. Also, we might want to put a deprecation notice in the console or something when it's used. It's good to provide advanced notice if we can.
There was a problem hiding this comment.
The plan according to the PR is to move away from transform and towards using reducer from what I understand. So it makes sense to add a console warning. Thanks 👍
There was a problem hiding this comment.
I'm not sure the best place for the warning. @acatl or @raingerber?
There was a problem hiding this comment.
I'm not sure if @acatl wants to deprecate transform or not - but if we do, I think it's enough to mention that in the readme (i.e. this will be deprecated in the future) because we would be providing a codemod to make the deprecation as easy as possible
packages/data-point/README.md
Outdated
| @@ -1510,6 +1510,42 @@ dataPoint.addEntities({ | |||
| ``` | |||
| </details> | |||
|
|
|||
| Note: We recommend using reducer as an alias for transform | |||
|
|
|||
| The following example produces the same result as its reducer counterpart: | |||
There was a problem hiding this comment.
Same comment about backticks around reducer.
…the list of entities that can b ViacomInc#167
…res that the reducer and transf n ViacomInc#167
| @@ -1462,24 +1462,30 @@ To customize type checking you may use a [Reducer](#reducers). If the reducer th | |||
| </details> | |||
|
|
|||
|
|
|||
| #### <a name="transform-entity">Transform Entity</a> | |||
| #### <a name="reducer-entity">Reducer Entity</a> | |||
There was a problem hiding this comment.
I noticed that this line is in another section of the readme:
for all entites except transform
do you mind replacing that sentence with:
for all entities except [Reducer](#reducer-entity)
?
…n for Reducer Entity
|
@paulmolluzzo Has victor addressed the changes that you requested? |
|
@raingerber yup, all good! Thanks @victorsingh! 🙌 |
…acomInc#209) - Added reducer alias to the documentation for transform entity - Updated .all-contributorsrc - Improved documentation - Updated Accumulator documentation for Reducer Entity closes ViacomInc#167
What: We want 'reducer' to be used as an alias for 'transform'
Why: Because a transform entity is just a cached reducer
How:
manager.addEntityType('reducer', EntityTransform)Was added to the built in entity types.
Checklist: