Support OneOf #38 #49
Conversation
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
- Coverage 88.94% 87.55% -1.39%
==========================================
Files 4 4
Lines 624 635 +11
==========================================
+ Hits 555 556 +1
- Misses 38 44 +6
- Partials 31 35 +4
Continue to review full report at Codecov.
|
alexeyneu
force-pushed
the
v2
branch
3 times, most recently
from
f6c3265
to
f98fccb
Compare
proofs/flatten.go
Outdated
| @@ -55,8 +55,12 @@ func (f *messageFlattener) handleValue(prop Property, value reflect.Value, saltV | |||
|
|
|||
| // Handle each field of the struct | |||
| for i := 0; i < value.NumField(); i++ { | |||
| var oneof_here bool | |||
There was a problem hiding this comment.
Could you rename this to oneOfField?
alexeyneu
force-pushed
the
v2
branch
9 times, most recently
from
8d0e0f2
to
d04898c
Compare
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
- Coverage 88.69% 88.56% -0.13%
==========================================
Files 3 3
Lines 575 586 +11
==========================================
+ Hits 510 519 +9
- Misses 36 37 +1
- Partials 29 30 +1
Continue to review full report at Codecov.
|
alexeyneu
force-pushed
the
v2
branch
2 times, most recently
from
6798cd3
to
3afb8d7
Compare
proofs/salts_test.go
Outdated
| @@ -28,7 +28,7 @@ func TestFillSalts(t *testing.T) { | |||
|
|
|||
| //Document with oneof fieldset | |||
There was a problem hiding this comment.
// Document with oneof fieldset
Space between double dashes and comment please
proofs/tree_test.go
Outdated
| err = doctree.Generate() | ||
|
|
||
| _, err = doctree.CreateProof("InexistentField") | ||
| assert.EqualError(t, err, "No such field: InexistentField in obj") |
There was a problem hiding this comment.
You probably don't need to test this here if it's tested elsewhere.
There was a problem hiding this comment.
The coverage report shows this line not being covered, is this correct? https://codecov.io/gh/centrifuge/precise-proofs/pull/49/diff?src=pr&el=tree#D6-97
In addition, it would be nice to add a similar test as this one here:
precise-proofs/proofs/tree_test.go
Line 143 in 3afb8d7
That would show that if valueC is set, then the tree would have: valueA, valueC as properties and if valueB is set it would only have valueA, valueB as properties?
proofs/tree_test.go
Outdated
| assert.Nil(t, err) | ||
|
|
||
| falseProof, err := doctree.CreateProof("valueB") | ||
| falseProof.Value = "Invalid" |
There was a problem hiding this comment.
this test doesn't test much and is covered in other test cases already
|
|
||
| proof, err := doctree.CreateProof("valueB") | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, ReadableName("valueB"), proof.Property) |
There was a problem hiding this comment.
Could you create a test for the following additional scenario: Try to create a proof for valueC when valueB is set
There was a problem hiding this comment.
And when both are nil create a proof for either one (or even both) when both are nil.
alexeyneu
force-pushed
the
v2
branch
2 times, most recently
from
0e1da40
to
03be8a5
Compare
|
We've changed how salts are handled in #43 and that code has now been merged. Could you resolve the conflicts please? |
|
it's coz scott agreed to pay part of money himself |
alexeyneu
force-pushed
the
v2
branch
3 times, most recently
from
959f604
to
f97e5d4
Compare
|
alexeyneu
force-pushed
the
v2
branch
9 times, most recently
from
33a726c
to
af0db42
Compare
|
@alexeyneu thank you for resolving the conflicts. @vimukthi-git will review the PR and test it locally. We might still have some smaller feedback but given that is all addressed we'll merge it and pay out the bounty. |
Fixes #38