object: Improve testing and increase coverage for Object type
#634
Conversation
Follow 80da1c6. Signed-off-by: Leonard Lyubich <[email protected]>
Follow 8e8ebc7. Signed-off-by: Leonard Lyubich <[email protected]>
Signed-off-by: Leonard Lyubich <[email protected]>
Session token is now expected to be signed, and attributes must be unique. Signed-off-by: Leonard Lyubich <[email protected]>
cthulhu-rider
changed the title
Object type
cthulhu-rider
force-pushed
the
object
branch
3 times, most recently
from
f9974ba to
3f19c06
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #634 +/- ##
==========================================
+ Coverage 56.33% 57.44% +1.11%
==========================================
Files 164 164
Lines 22403 22486 +83
==========================================
+ Hits 12620 12917 +297
+ Misses 9396 9175 -221
- Partials 387 394 +7 ☔ View full report in Codecov by Sentry. |
cthulhu-rider
requested review from
roman-khimov,
smallhive and
carpawell
as code owners
| @@ -38,7 +38,7 @@ func (o *Object) CalculateAndSetPayloadChecksum() { | |||
|
|
|||
| // VerifyPayloadChecksum checks if payload checksum in the object | |||
| // corresponds to its payload. | |||
| func (o *Object) VerifyPayloadChecksum() error { | |||
There was a problem hiding this comment.
again: nspcc-dev/.github#29. my biggest concern is why we continue doing this. it is questionable (we do not have the answer on this) but still, we see such commits. we may "fix" it here and have thousands of structs that do not follow this rule. so what's the point?
There was a problem hiding this comment.
We're waiting for benchmarks to show copying happens and matters. Then we'll change it back.
There was a problem hiding this comment.
mostly, existing ro methods are defined on types, not pointers. And some methods like this differ unreasonably. Im tryin to stick them to the most expected view basically. If there will be a particular reason to have a pointer-based method - we'll add it. Currently, there is no reason to pass pointer for payload verification
|
|
||
| return true | ||
| x.err = dst.ReadFromV2(objv2) | ||
| return x.err == nil |
There was a problem hiding this comment.
immo this is kinda stylish thing: i would prefer to always keep a new line before every return. it used to have it, but now it does not. white space change as @roman-khimov would say. do not insist
There was a problem hiding this comment.
It's a bit different, not a pure whitespace change since the logic is changed in these blocks. Works for me both ways.
There was a problem hiding this comment.
not an occasional whitespace change like #634 (comment)
There was a problem hiding this comment.
not an occasional whitespace change
i understand but this is also not a new functionality just the old one was changed. i try not to drop/add new lines in such cases even if i have a different style
With 340c369, container/object ID setter/getter accepts/returns corresponding ID type by value. in a structural sense, the owner ID of `user.ID` type is no different. However, its setter and getter were built over pointer to the value. Now the interface of all identifiers in the object is uniform: - setter accepts ID by value; - getter returns ID by value; - if unset, getter returns zero instance. Signed-off-by: Leonard Lyubich <[email protected]>
The owner is mandatory and must never be zero. Signed-off-by: Leonard Lyubich <[email protected]>
Some kind of regression from ade8822: `Version` method started to return pointer to zero instance instead of nil. Since the absence of a field and an explicitly set zero are different cases, the behavior was initially more correct, so this returns it. Signed-off-by: Leonard Lyubich <[email protected]>
Signed-off-by: Leonard Lyubich <[email protected]>
Signed-off-by: Leonard Lyubich <[email protected]>
cthulhu-rider
force-pushed
the
object
branch
from
3f19c06 to
7d16474
Compare
it remains to add tests to the verification fields