Add offset and length to pycurl #95
Conversation
|
Thanks @mlouielu ! I will take a look at this over the weekend. |
todofixthis
left a comment
todofixthis
left a comment
There was a problem hiding this comment.
Hey @mlouielu this is looking pretty good!
I changed the target branch to develop, and I requested a few changes; mostly minor.
| 'offset': offset, | ||
| 'length': length, | ||
| }, | ||
| ) |
iota/crypto/pycurl.py
Outdated
| offset += HASH_LENGTH | ||
|
|
||
| def squeeze(self, trits): | ||
| def squeeze(self, trits, offset=0): |
There was a problem hiding this comment.
Should squeeze also have a length argument?
In practice, it seems that this argument is always HASH_LENGTH, but the other libraries do support it. Examples:
There was a problem hiding this comment.
Added, but in real case, this doesn't use without 243 (HASH_LENGTH)
test/crypto/pycurl_test.py
Outdated
| from unittest import TestCase | ||
|
|
||
| from iota import TryteString | ||
| from iota.crypto.pycurl import Curl |
There was a problem hiding this comment.
Could you change this to from iota.crypto import Curl? That way, I can repurpose these tests for the C extension.
More info:
|
|
||
|
|
||
| class TestCurl(TestCase): | ||
| def test_correct_first(self): |
There was a problem hiding this comment.
Could you add a docstring to each unit test, describing the situation it is testing? The test names are a good start, but a little extra documentation describing the context for each test will go a long way.
| for i in range(6): | ||
| curl.reset() | ||
| curl.absorb(trits, i * 243, (i + 1) * 243) | ||
| curl.squeeze(trits, i * 243) |
There was a problem hiding this comment.
This test looks very specific. Is it mimicking a real-world use case?
Unit tests also serve as documentation; if you add some comments explaining what real-world situation this test is simulating, it will help users to better understand what they can accomplish with the library.
There was a problem hiding this comment.
This only use in ISS and ISSInplace, add the source code link in docstring.
|
Hey @mlouielu I feel like this PR is almost ready to merge. I've requested a few changes, but they're mostly minor stuff, documentation and such. Let me know if you have any questions, or if I can provide additional info. |
|
I'll find some time these days to make sure this PR fits your guide. |
|
Thanks Louie! |
pycurl.Curl.absorb:
Add offset and length
pycurl.Curl.squeeze:
Add offset
sequeeze not added with length, is because it will only
copy one hash to trits, therefore no need to add length
mlouielu
force-pushed
the
add_pycurl_offset_length
branch
from
f08fc0a to
0245bc8
Compare
|
Hey Louie! Thanks for working on this! I will take a look this weekend and follow up. |
todofixthis
left a comment
todofixthis
left a comment
There was a problem hiding this comment.
Looks great! Thanks Louie!!
…set_length Add offset and length to pycurl
2 participants
Issue: #92
pycurl.Curl.absorb:
Add offset and length
pycurl.Curl.squeeze:
Add offset