-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add TransactWriteItems #48
Conversation
@samritchie This develops bartelink#3 Will keep chipping away adding tests but would appreciate your thoughts on where to draw the line - ultimately I'm satisfied the basic functionality of the exception filter and the condition checks etc are correct. After that, the incremental value of validating arbitrary combinations of conditions and individual |
2f9c754
to
e3d3db2
Compare
@samritchie I can't think of anything else to do so from my point of view this is ready for final review - let me know if you can see anything that needs doing (in particular, the README is more a braindump than polished tech writing!) if not, I'll integrate test it when you get a chance to merge and release |
1448129
to
45fe9ba
Compare
Thanks @bartelink, I'll go through this today - I’m still recovering from the plague and my brain is a bit slow at the moment |
Thanks - I know the feeling so doubly appreciate any time you can carve out in that case. For reference, here's where it all slots into Equinox (am I glad I have tests over that!) https://github.com/jet/equinox/blob/6a23d9e26aab33b027d7694502415a552e9ce5bb/src/Equinox.DynamoStore/DynamoStore.fs#L417-L468 (And if you think no reasonable person could love that code, contrast it with https://github.com/jet/equinox/blob/6a23d9e26aab33b027d7694502415a552e9ce5bb/src/Equinox.CosmosStore/CosmosStore.fs#L362-L469, which definitely took more development effort and tight-rope walking!) |
Nudge @samritchie - hoping to release a beta of jet/equinox#321 vs having to hand-build some alpha... Even an |
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'm happy to merge & release this - there’s certainly value here for the single-table crowd.
However this library doesn’t cater particularly well for that pattern & I don’t know how many of those users there are. I expect more than one person will end up wasting time trying to work out out how to do TransactWrite across multiple tables - it feels a little incomplete.
The docs are a bit wordy and are heavy on third party links - have put some comments in there but let’s tidy that up when we have time.
|
||
do! table.TransactWriteItems requests | ||
|
||
let! itemFound = table.ContainsKeyAsync (table.Template.ExtractKey item) |
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'm not overly enthusiastic about the use of things like template.ExtractKey
in the tests, purely because new users often look at tests for usage guidance, and I view the record template as internal-ish. Don’t do anything with this for now though, I will have a more general review of the tests.
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.
Will be interested to see what your alternative is; are you suggesting having a property on the record that calls TableKey.Combined
explicitly?
Thanks for releasing, much appreciated. Your comments are on the money; will circle back on them in the coming days... |
Wraps the
TransactWriteItems
APIArgumentOutOfRangeException
Check
+ unconditionalPutItem
pairAdds a
Precondition.CheckFailed
for trapping normal precondition check fails