search

cnabio / cnab-go

A Go implementation of CNAB Core 1.0
MIT License
69 stars 37 forks source link

feat(claim): add Result validation #190

Closed vdice closed 4 years ago

vdice commented 4 years ago

Note for reviewers: Do we want to modify the function signature for the Update method on a Claim to return an error? We can then check to be sure the supplied action and status form a valid Result; if not, we would return an error.

radu-matei commented 4 years ago

Yes, I think Update should indeed return an error.

vdice commented 4 years ago

Yes, I think Update should indeed return an error.

@radu-matei I was also thinking about this afterwards... while doing so, should we adjust the arg set from action, status string to res Result?

radu-matei commented 4 years ago

Not sure about that - interestingly enough, we never update the Message field from a Result, so we should at least add that as well.

The question with respect to passing a Result is whether the caller has the object constructed beforehand, or it needs to construct it to call Update. Either way, I don't have a strong preference.

silvin-lubecki commented 4 years ago

This PR needs a rebase 🐯

vdice commented 4 years ago

Rebased. Note, however, that the test of a claim against the current claim schema from https://cnab.io will fail. @carolynvs will be working on changes to the Claim/Result structures per the updated schema.

vdice commented 4 years ago

I've pushed a commit to skip the claim schema test for now. Issue to re-enable (and add the rest of the implementations under test) is being tracked in https://github.com/cnabio/cnab-go/issues/194

vdice commented 4 years ago

@radu-matei I am going to hold off on making any changes to the Update method as I am predicting it will change further with recent amendments to the spec (splitting claim from result, etc.)