Skip to content
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

Reducer function acc value acc #172

Merged
merged 11 commits into from Jan 25, 2018

Conversation

acatl
Copy link
Collaborator

@acatl acatl commented Jan 23, 2018

What: #117

Why: #117

How: changing FunctionReducer API

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added username to all-contributors list

you might want to open this PR in FireFox

Acatl Pacheco added 3 commits January 22, 2018 15:38
(acc) -> (input, acc) codemods provided under data-point-codemods

BREAKING CHANGE: every reducer function needs to be changed to accept the first parameter as the
reducer's input, use codemods provided

closes ViacomInc#117
@coveralls
Copy link

coveralls commented Jan 23, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 3d1b5ef on acatl:reducer-function-acc-value-acc into ee4cfeb on ViacomInc:master.

Copy link
Collaborator Author

@acatl acatl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:)

@@ -0,0 +1,160 @@
const util = require('util')
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't be too nitpicky here!! :)

@@ -0,0 +1,17 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this test to check against the API we are exposing on dataPoint instances

// supports currying
manager.resolve = Transform.resolve(manager)
// does not support currying
manager.transform = _.partial(Transform.transform, manager)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I debated between adding curry to transform, I think we should leave it as is, no need to add more breaking changes, hope u guys agree

@@ -21,6 +21,10 @@ beforeAll(() => {
})
})

test('api', () => {
expect(Object.keys(instance)).toMatchSnapshot()
Copy link
Collaborator Author

@acatl acatl Jan 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this tests DataPoint instance's API

@@ -102,8 +102,8 @@ const app = new express()
Service.create({
// add DataPoint entities
entities: {
'entry:HelloWorld': (acc) => 'Hello World!!',
'entry:Greet': (acc) => `Hello ${acc.locals.params.person}!!`
'entry:HelloWorld': (value, acc) => 'Hello World!!',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are many instances where you will see this (value, acc) or even (value, acc, next) I did a search replace, there were too many specific cases where sometimes acc was used other than acc.value, and the codemods cant handle many of our use cases on tests, so decided to just refactor all to be (value, acc, [next])

/* eslint-disable */
/* eslint-disable prettier */

// prettier-ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are the linters ignored?

Copy link
Collaborator Author

@acatl acatl Jan 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good question indeed!!!

the reason is that codemods are not perfect, when code gets generated from AST back to source code, they sometimes get formatted, in some cases it will add a semicolon at the end of the expression. Im leaving up to the developer to run prettyfier or anything of the sort to match his current style.

so back as to why: these above are testfixtures, which are used by the codemod test I made, when its ran, it might add ; semicolons, or change the format slightly. at that point its all good, the issue is that when we do yarn run commit it will run prettier on the code and remove the colons, and then it will execute the tests, and the tests will fail....

so, to avoid this, I ask prettier to ignore the block, so it doesn't get prettified by our precommit hook.

drop-mic

@@ -11,19 +11,19 @@ const request = require('supertest')
const logger = require('./logger')
logger.clear()

describe('create - inspect middleware', () => {
describe.only('create - inspect middleware', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove only

)
})

test('transform - execute with 3 argumennts', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arguments

done(null, result)
return function (value) {
const methodArguments = [value].concat(partialArguments)
return method.apply(null, methodArguments)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will reducify still be useful now that value is passed separately from the accumulator?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes because adds a closure to pass the value as the first parameter, which prob should be documented more, but yes still makes sense

})

test('reducify specificy keys', done => {
test('reducify specificy keys', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specific?

const value = _.set(context.value, key, val)
next(null, value)
module.exports.addKeyValue = (key, val) => value => {
// TODO: check why this is mutating object, dont believe it should
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this something we need to address? _.set does modify the object...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, not part of this PR but yeah when I was refactoring found that and a couple more, reason why I left that note

const value = utils.set(initialValue, `qs.${key}`, val)
next(null, value)
module.exports.addQueryVar = (key, val) => value => {
// TODO: check why this is mutating object, dont believe it should
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this something we need to address?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as on packages/data-point/test/utils/reducers.js

@@ -0,0 +1,203 @@
Copyright 2017 Viacom Inc.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,25 @@
{
"name": "data-point-codemods",
"version": "1.7.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we start this at v0.1.0 or v1.0.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we use lerna and the way we deploy, we deploy the same version for each package on every deployment

https://github.com/lerna/lerna#fixedlocked-mode-default

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ew, but OK.

: newParams
}

function replaceAccValueReferences (node) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand this right: If the function uses acc to reference the accumulator, it'll change, but if they've used a or accumulator then this won't work? Is it possible to only check the arity and update accordingly?

Also, what happens with something like this?

// increments
dataPoint.transform(a => a.value++)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • it complicates the code, not that easy to tell which are reducers and which arent,
  • .transform I have a check to ignore those handlers

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fair points. Thanks for clarifying.

@@ -16,7 +16,7 @@ describe('create - all middleware', () => {
beforeAll(() => {
const options = {
entities: {
'transform:hello': acc => ({
'transform:hello': (value, acc) => ({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value should be input to match all the docs and codemod.

I noticed this a few places (mostly in test files), if you don't mind changing them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its all over the place, and its not that easy change, it does make sense I agree, but would rather leave it for a second round where we actually check on which reducers makes sense to even pass acc at all, since I couldn't run the codemods on the data-point code itself aside from examples, it was 1 day of search and replace one by one

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, fair enough. Fixing something like this is simple enough to do as a first PR, so I'm cool with leaving it for now.

paulmolluzzo
paulmolluzzo previously approved these changes Jan 24, 2018
Copy link
Collaborator

@paulmolluzzo paulmolluzzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🎉

Left a few comments though none are really major.

Approving even though there's a merge conflict. If the merge conflict is complex and you think it's worth another review I can do that.

Also, we have a drop in test coverage. ☹️ There's one line in packages/data-point-codemods/transforms/reducer-args-acc-to-val-acc.js that isn't tested in all scenarios. Not sure if we care enough but branches are not 100% covered anymore.

@acatl acatl closed this Jan 24, 2018
@acatl acatl reopened this Jan 24, 2018
})
.size()

let injectAcc = node.value.params.length === 2 || accReferences > 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const?

paulmolluzzo
paulmolluzzo previously approved these changes Jan 24, 2018
Copy link
Collaborator

@paulmolluzzo paulmolluzzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Will make upgrading so much easier!

Copy link
Contributor

@raingerber raingerber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great change!

Copy link
Collaborator

@paulmolluzzo paulmolluzzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@paulmolluzzo paulmolluzzo merged commit 8ac4f37 into ViacomInc:master Jan 25, 2018
@acatl acatl deleted the reducer-function-acc-value-acc branch May 4, 2018 02:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants