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
Support for conditional upsert #63
Conversation
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.
✅ A review job has been created and sent to the PullRequest network.
@prashant-shahi you can click here to see the review status or cancel the code review job.
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.
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.
👍 this looks good other than the issue of accessing a function vs calling a function I pointed out.
Reviewed with ❤️ by PullRequest
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.
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.
Reviewed 22 of 26 files at r1, 6 of 6 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @mangalaman93, @prashant-shahi, and @pullrequest[bot])
protos/api.proto, line 170 at r2 (raw file):
} // vim: noexpandtab sw=2 ts=2
Just to confirm: this file is an identical copy of the api.proto in the main dgraph repo, right?
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.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mangalaman93, @prashant-shahi, and @pullrequest[bot])
a discussion (no related file):
It seems like linReads is a separate unrelated API change, and if such it should be done in a separate PR.
OK to commit it as is since 1.1 release is so close, next time let's try to have a PR per feature.
Overall looks good to me.
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.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mangalaman93, @paulftw, and @pullrequest[bot])
a discussion (no related file):
Previously, paulftw (Paul Korzhyk) wrote…
It seems like linReads is a separate unrelated API change, and if such it should be done in a separate PR.
OK to commit it as is since 1.1 release is so close, next time let's try to have a PR per feature.Overall looks good to me.
oh, sure. will do it from now on.
protos/api.proto, line 18 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Camel case with an initial capital is known as Pascal case.
Done.
protos/api.proto, line 19 at r1 (raw file):
Previously, pullrequest[bot] wrote…
underscore_separated_names is known as Snake case
Done.
protos/api.proto, line 26 at r1 (raw file):
Previously, pullrequest[bot] wrote…
Do these commented out lines still need to be part of the code? If yes - probably make them all one multiline comment, or multiple single-line comments.
Right now there are multiple multiline comments of 1 line each.
Done.
protos/api.proto, line 170 at r2 (raw file):
Previously, paulftw (Paul Korzhyk) wrote…
Just to confirm: this file is an identical copy of the api.proto in the main dgraph repo, right?
Yes. It is identical. Just that few comments have been either altered and removed.
Adding support for conditional upsert.
This change is