pydgraph
pydgraph copied to clipboard
dgraph-io
adding txn.upsert rough draft
This is a rough draft, a place to start a conversation. Hopefully this can be useful in upstream
This change is 
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
I actually think the wrapper has three major advantages (on top of the normal ease of use) a) its easier for new users to quickly find. Personally, I had to search through the unit tests to figure out how to use the new upsert stuff, and if I was newer to dgraph I am not sure if I would have been able to find the feature as quickly as I did. b) It fits within the existing design, txn.mutate and txn.query have wrappers so logically a new type of interaction should have its own new wrapper. c) [could be related to a] it makes the code more readable, if I was a new contributor to a team that uses dgraph it would be immedietly clear what txn.upsert().
Ultimately, this is your guys' product so you get the final say, this is just my two cents. As always, I appreciate the continuous support and quick responses.
query and mutate exist as wrappers for backwards-compatibility reasons. My main issue with this wrapper is that we try to get the different clients to be as close as possible. So if the wrapper is useful, we should decide whether to bring it on every client. I'll bring up this discussion with the rest of the team to see what they think.
If you have specific complains about the upsert documentation, could you open an issue in the dgraph repo?
query and mutate exist as wrappers for backwards-compatibility reasons. My main issue with this wrapper is that we try to get the different clients to be as close as possible. So if the wrapper is useful, we should decide whether to bring it on every client. I'll bring up this discussion with the rest of the team to see what they think.
That's a great point, keeping the APIs equivalent is something I hadn't considered. I'll update the PR with the information you all asked for later this week.
Thanks for the quick review!
@Kingloko We do not plan to add this in the library but we could add this as a test or an example. If you could raise a PR with that, would be happy to look at it.
