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

Implement ConditionBuilder as a recursive evaluation engine. #108

Merged
merged 1 commit into from
Feb 6, 2017

Conversation

nicks
Copy link
Contributor

@nicks nicks commented Jan 18, 2017

Hello @andrew3886, @xiao,

Please review the following commits I made in branch 'nicks/conditions'.

41374fc (2017-01-17 20:24:26 -0500)
Implement ConditionBuilder as a recursive evaluation engine.
This doesn't change the existing API. This just clears
the way to support arbitrarily complex DynamoDB expressions.

7993582 (2017-01-17 17:24:46 -0500)
Move Dynamite over to the new UpdateExpression API

R=@andrew3886
R=@xiao

@nicks
Copy link
Contributor Author

nicks commented Jan 18, 2017

note this depends on #107

@nicks
Copy link
Contributor Author

nicks commented Jan 25, 2017

ping?

@nicks
Copy link
Contributor Author

nicks commented Feb 1, 2017

cc @kellyellis @kylewm this is the attribute-filter PR that i told you about yesterday, it basically copies Hopper's approach of having a struct representing the filter.

@kellyellis
Copy link

👀

Copy link

@kellyellis kellyellis left a comment

Choose a reason for hiding this comment

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

I'll take a deeper look later today, wanted to get in some small requests for documentation :)
So far looks good, based on skimming


/**
* @param {Object=} opt_conditions
* @param {Object=} opt_exprs

Choose a reason for hiding this comment

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

Might help to document a little about what this Object looks like

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, agreed. improved the docs in a couple places. also i just removed this argument, because i think it's probably more confusing than useful.

* @param {Array.<ConditionBuilder|ConditionJunction>} conditions
* @return {ConditionJunction}
* Creates a new Conditional expression with an operator (AND, EQ, etc)
*/

Choose a reason for hiding this comment

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

I realize they aren't required here, but @param JSDoc would make these functions easier to understand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. i defined some better types for these that will hopefully make it clearer and more constrained.

This doesn't change the existing API. This just clears
the way to support arbitrarily complex DynamoDB expressions.
Copy link

@kellyellis kellyellis left a comment

Choose a reason for hiding this comment

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

LGTM

@nicks nicks merged commit 92ffe87 into master Feb 6, 2017
@nicks nicks deleted the nicks/conditions branch February 6, 2017 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants