fix (CoRIM Extensions) : Incorrect Assumptions on Measurement Values Map validity #141
Conversation
…ement values Signed-off-by: Priyanshu Thapliyal <[email protected]>
Priyanshuthapliyal2005
force-pushed
the
fixExt
branch
from
183b423 to
e6b0fdc
Compare
comid/measurement.go
Outdated
| @@ -436,7 +437,10 @@ func (o Mval) Valid() error { | |||
| o.UEID == nil && | |||
| o.UUID == nil && | |||
| o.IntegrityRegisters == nil { | |||
| return fmt.Errorf("no measurement value set") | |||
|
|
|||
| if o.Extensions.IsEmpty() { | |||
There was a problem hiding this comment.
Why not include this in the conjunction? I don’t see a reason for the nested if.
There was a problem hiding this comment.
I uses nested if statements for simpler view of the conditions. However, if using a conjunction is preferred for better readability, I can change it accordingly.
There was a problem hiding this comment.
I would prefer it. I don’t think there’s anything about this condition that needs special attention
There was a problem hiding this comment.
Thanks for the feedback! it's cleaner with conjunctions. I've updated the code
Priyanshuthapliyal2005
force-pushed
the
fixExt
branch
from
711e3d6 to
2192da5
Compare
…surement values Signed-off-by: Priyanshu Thapliyal <[email protected]>
Priyanshuthapliyal2005
force-pushed
the
fixExt
branch
2 times, most recently
from
5a0ffef to
c32f413
Compare
comid/measurement.go
Outdated
| @@ -424,6 +424,7 @@ func (o Mval) MarshalJSON() ([]byte, error) { | |||
| } | |||
|
|
|||
| func (o Mval) Valid() error { | |||
| // Check if all base types are nil and Extensions are not nil | |||
There was a problem hiding this comment.
Please move this to be a method comment, “Valid returns true if all […]” since Valid is exported but doesn’t have a comment.
There was a problem hiding this comment.
I have updated the Valid method to include a comment and moved the check for empty extensions as suggested.
Priyanshuthapliyal2005
force-pushed
the
fixExt
branch
2 times, most recently
from
67fba18 to
8830b3c
Compare
Signed-off-by: Priyanshu Thapliyal <[email protected]>
Priyanshuthapliyal2005
force-pushed
the
fixExt
branch
from
8830b3c to
748c338
Compare
Fixes #134