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

feat(data-point): Remove predefined hash and collection execution order #218

Merged
merged 12 commits into from
Feb 21, 2018

Conversation

raingerber
Copy link
Contributor

What: closes #73, and has a codemod because it's a breaking change

Why: Makes it easier to understand hash and collection entity definitions without consulting the readme, because the execution order for "modifiers" is always defined by the order of the compose array

How: Refactored parse-compose.js

Checklist:

  • Has Breaking changes
  • Documentation
  • Tests
  • Ready to be merged

Armstrong added 4 commits February 14, 2018 13:54
…process arrays

rename getTypeModifier to normalizeTypeCheckSource
…ection

to use more than one modifier, use a compose array

BREAKING CHANGE: No longer possible to use multiple modifiers without a compose array

ViacomInc#73
parseCompose.validateCompose(entity.id, composeSpec, modifierKeys)
if (composeSpec.length) {
entity.compose = createCompose(composeSpec)
const compose = parseCompose.parse(id, modifierKeys, spec)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored parseCompose so that all the functionality is contained by the parse function (instead of needing to individually call validateComposeModifiers, parse, and validateCompose)

@coveralls
Copy link

coveralls commented Feb 19, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling d728aaa on raingerber:hash-and-collection-execution-order into 756ba05 on ViacomInc:master.

map: '$a',
find: '$a',
filter: '$a'
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is no longer a valid object, because map / filter / find would need to be inside compose

value: {},
'inputType': 'object',

compose: [{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

jcodeshift adds the empty line before compose. not sure how to prevent that...

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's interesting, and hard to ping down. This is the closest information I can find to it. benjamn/recast#147

Is this a big deal to try to "fix" or are we OK with leaving it? I don't think it's worth the effort to solve this as any source files changed by the codemod might go through a linter/formatter by users anyway and leave/change these empty lines on their own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it's not worth trying to fix

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, yeah, I agree.

execute('collection', ['filter', 'map', 'find'])
execute('hash', ['omitKeys', 'pickKeys', 'mapKeys', 'addValues', 'addKeys'])

return root.toSource()
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the codemod for changing access to the acc root we have a means of checking the quote style to preserve it. Do you think it's worth adding that here? If so, it might be worth pulling the function that's used into a utility while we're at it.

// As Recast is not preserving original quoting, we try to detect it,
// and default to something sane.
// See https://github.com/benjamn/recast/issues/171
// and https://github.com/facebook/jscodeshift/issues/143
// credit to @skovhus: https://github.com/avajs/ava-codemods/pull/28
const quote = detectQuoteStyle(j, root)
return root.toSource({ quote })

Copy link
Collaborator

@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.

thanks for this change!

@acatl acatl merged commit 8fc75c1 into ViacomInc:master Feb 21, 2018
format(
'Entity "%s" has invalid modifiers, when using multiple keys they should be added through the "compose" property.\nValid modifiers: %s.\nFor more info: %s',
entityId,
specKeys.join(', '),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this be validKeys instead of specKeys.join(', ')?

@raingerber raingerber deleted the hash-and-collection-execution-order branch February 26, 2018 20:20
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.

Remove predefined execution order for hashes and collections
4 participants