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
Remove the need to pass Put.expectedContent()
#6438
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #6438 +/- ##
============================================
+ Coverage 83.23% 84.54% +1.31%
Complexity 537 537
============================================
Files 909 909
Lines 36033 36033
Branches 3220 3220
============================================
+ Hits 29991 30465 +474
+ Misses 4907 4399 -508
- Partials 1135 1169 +34
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 24 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Put
+ Delete
Put.expectedContent()
7199cd5
to
a0e1af9
Compare
a0e1af9
to
8423025
Compare
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.
LGTM overall 👍 Minor comments, can be addressed in a follow-up PR.
.toBranch(branch)) | ||
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessage("Key 't3' does not exist, but Put-operation has expectedValue"); | ||
.hasMessage("New value for new must not have a content iD"); |
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.
nit: the message looks odd 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.
Why? It's trying to create an IcebergTable with a content-ID, which is not allowed
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.
Maybe quote the key in the message? This particular message is just hard to read :)
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.
Also iD
starts with a lower case ;)
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.
`expectedContent` was originally introduced for global-state, which has been removed a while ago. The field `expectedContent` is still present and used for REST API v1, but excluded for REST API v2.
8423025
to
0843d07
Compare
expectedContent
was originally introduced for global-state, which hasbeen removed a while ago.
The field
expectedContent
is still present and used for REST API v1,but excluded for REST API v2.