-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Operand Implementation Pull Request #1421
Conversation
type OperandBuilder interface { | ||
BuildOperand(AliasList) (Expression, error) | ||
ListOperand() (AliasList, error) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest this being:
type OperandExpression struct {
OpType
Key string
Value *dynamodb.AttributeValue
}
type OperandBuilder interface {
BuildOperand() (OperandExpression, error)
}
|
||
const ( | ||
// EqualCond will represent the Equal Clause ConditionBuilder | ||
EqualCond ConditionMode = iota + 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we starting at 1 here?
|
||
switch c.Mode { | ||
case EqualCond: | ||
ret.fmtExpr = "$c = $c" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably add some comments to this function about what $
means
name, value, children int | ||
}{} | ||
|
||
//index := [3]int{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment
} | ||
|
||
var alias string | ||
switch expr.Expression[i+1] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this case ever happen i == len(expression)-1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if an AST would be better here as opposed a fmt string
index.children++ | ||
} | ||
expr.Expression = expr.Expression[:i] + alias + expr.Expression[i+2:] | ||
i += len(alias) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably have a default case that returns an error saying case not supported
* Initial commit * write Base32Encoding test cases * Implement numeric encoding instead of Base32 * Minor changes to satisfy golinter * Implement unexported tree structure * write tests * Finish basic tests * Use sprintf style aliasing * Add BuildExpression method to ConditionBuilder * Make change based on inline PR * Add name to tests, comments for clarity, more index panic coverage
For changes to files under the
/model/
folder, and manual edits to autogenerated code (e.g./service/s3/api.go
) please create an Issue instead of a PR for those type of changes.If there is an existing bug or feature this PR is answers please reference it here.