Feature: named structures #169
Conversation
|
One question, pylint seems to be unable to ensure that |
|
|
||
| # pylint: disable=no-member,protected-access,blacklisted-name,missing-docstring | ||
|
|
||
| def test_named_struct_roundtrip(): |
There was a problem hiding this comment.
These might be a good candidate to use Hypothesis on, where we can try a wide variety of input values. I'll give it a try and see what we get.
There was a problem hiding this comment.
Oh, that's a great idea, I had forgotten about Hypothesis. Agreed, though, seems like a really good candidate for finding test cases.
There was a problem hiding this comment.
As a sneak peek:
from hypothesis import given
import hypothesis.strategies as st
@given(st.integers(min_value=0, max_value=0x7FFF*2+1), st.integers(min_value=0, max_value=0xFF))
def test_named_struct_roundtrip(var1, var2):
class Foo(NamedStruct):
a = Field('H')
padding = Padding(12)
b = Field('B')
foo = Foo(a=var1, b=var2)
assert Foo.unpack(foo.pack()) == fooSeems to work pretty well!
There was a problem hiding this comment.
That's awesome, love that it works out for this case. That may help us write other unit tests in the future, too. For this PR, maybe it makes sense to use a string strategy for StringField instances?
There was a problem hiding this comment.
I believe so; I'm going to give it a try and see how it works.
2 similar comments
|
I wonder if we should pull this out into its own project. I can handle that if we want. Thoughts? |
|
I think that could be a good idea, there are quite a few instruments that i have now that require their own parser/unpack-er for the results. Could be nice to collect these all in one place... |
|
Yeah, the code in this PR is pretty generic, and I imagine its useful for all the different instrument's that you're working on. Heck, maybe even useful to other people 😲 |
|
In particular, the named struct pattern has come up before in parsing data returned by instruments. Apparently, using HDF5 is just too straightforward and useful... |
|
That would just make too much sense! And after dealing with things like tcubes... |
|
Revisiting this, would it make sense to include the feature in IK directly for now, possibly splitting it out into its own project later? The Python packaging infrastructure isn't especially great for micropackages (this might be a good thing, given left-pad), such that unless we're splitting out more than just named structures, the cost of a new package doesn't seem justified yet. Anyway, that's just my 2¢... |
|
Seems as though the build is still getting stuck on #174. I'll lock |
|
I think the coverage decrease is erroneous, since it's concentrated on files outside the scope of this PR, but I'll investigate further. |
|
I'm pretty happy with this PR now. Is there anything you'd like me to add, @scasagrande? Thanks! |
|
Locking numpy to I still feel that this should at some point probably be its own repo, but I'll worry about that on my own time in the future. Otherwise I'm good with moving forward with this. |
|
Also, I need to fix my github notifications... |
|
That's a good point, I'll change the version to |
In developing tests for #167, I finally got tired of packing and unpacking opaque
struct.Structformat strings and wrote a helper class to fix that. This helper class allows for referring to named fields of C-style structures, instead of having to remember the order of fields in aStructformat string. Though this new class doesn't yet support arrays other thanchar[], it should already be useful for applications such as the HW info response from APT devices.