Make encode method generalized for ComputedType
#629
Conversation
ahangsu
force-pushed
the
generalized-encode-for-computed-type
branch
from
a892314
to
994d9bb
Compare
ahangsu
force-pushed
the
generalized-encode-for-computed-type
branch
from
44318bb
to
76e6b58
Compare
| ignoreNext = 0 | ||
| for i, typeAfter in enumerate(value_types[index + 1 :], start=index + 1): | ||
| lastBoolStart = 0 |
There was a problem hiding this comment.
It's hard to tell -without a deep dive- whether this new version is functionally equivalent. If I understand correctly (???), there is currently no unit test that covers this code so we can't be 100% confident of correctness. If a unit test is added, (but probably that should be in the parent PR), then this would allay my concerns.
As an aside, I'm already thinking of starting some nightly tests. It might be a good opportunity to add python's coverage tool to such tests: https://github.com/nedbat/coveragepy
There was a problem hiding this comment.
My apologies, I am hacking through these APIs without a same set of unit test coverage, so I guarantee there will be a same set of unit test for functional equivalence.
The side note for coveragepy also makes sense to me, maybe not going into this PR, but as a separate one. The expected result should be somehow like go-algorand one, i.e., coverage +/- lines?
There was a problem hiding this comment.
Regarding coverage testing in C.I., definitely not this PR. I've created issue #630 as a placeholder to discuss this.
I see that __prototype_encoding_store_into() is being tested for the case when the output parameter is present from pre-existing tests that applied to store_into(). I'm not seeing where the case of empty output parameter is being tested. Since this is very relevant to (or even exactly) the functionality being refactored, we can't have confidence -from testing at least- that the refactoring is correct. So I think before merging this child PR we should add more coverage to #624.
There was a problem hiding this comment.
The refactoring of code is straightforward, which is against the old version of _index_tuple, by changing return item according to the existence of output: BaseType | None.
I added the testcases for tuple elements and array elements anyways, and will add some testcases for _GetAgainstEncoding, but I don't anticipate I have bandwidth for coverage adding in #624, and I hope @barnjamin can help on that one.
ahangsu
force-pushed
the
generalized-encode-for-computed-type
branch
from
9f3e752
to
33c3e87
Compare
No description provided.