DM-20019: Fix pickling of String Fields #468
Conversation
| @@ -75,6 +75,17 @@ class Access final { | |||
| /// @internal Access to the private Key constructor. | |||
| static Key<Flag> makeKey(int offset, int bit) { return Key<Flag>(offset, bit); } | |||
|
|
|||
| /// @internal Access to the private Key constructor. | |||
| static Key<std::string> makeKeyString(int offset, int size) { | |||
| return Key<std::string>(offset, FieldBase<std::string>(size)); | |||
There was a problem hiding this comment.
As I understand it you don't need the explicit construction of FieldBase, and the code is more readable without it (see makeKey<Flag>, above).
There was a problem hiding this comment.
I guess you could merge makeKeyString, makeKeyArray, and makeKey<Flag> into a single template, but then you'd need to include explicit arguments in all your calls (e.g., makeKey<std::string>(1, 42) or makeKey<Array<int>>(1, 42)). Not sure that's much of an improvement, especially if you need to hunt down the existing flag calls. 😞
| template <typename U> | ||
| static Key<Array<U>> makeKeyArray(int offset, int size) { | ||
| return Key<Array<U>>(offset, FieldBase<Array<U>>(size)); | ||
| } |
There was a problem hiding this comment.
Change the type parameter from U to T. By convention, U is used for templates that need to accept types other than some "primary" type (e.g., converting constructors), and it's a bit confusing when you only have one generic type.
| } | ||
|
|
||
| template <typename U> | ||
| void declareFieldSpecializations(PyField<Array<U>> &cls) { |
There was a problem hiding this comment.
As with the makeKey methods, you could make a template that covers both Array and string, and without the need for an explicit template argument.
| template <typename U> | ||
| void declareFieldSpecializations(PyField<Array<U>> &cls) { | ||
| cls.def(py::pickle( | ||
| [](Field<Array<U>> const &self) { |
There was a problem hiding this comment.
Consider using Class = Field<Array<U>> to reduce code duplication; see the style guide.
There was a problem hiding this comment.
I think there are few enough uses (3 total) that that definition doesn't help, and it gives another layer of indirection for someone looking at the code to have to follow.
| void declareKeySpecializations(PyKey<std::string> &cls) { | ||
| declareKeyAccessors(cls); | ||
| cls.def_property_readonly("subfields", [](py::object const &) { return py::none(); }); | ||
| cls.def_property_readonly("subkeys", [](py::object const &) { return py::none(); }); |
There was a problem hiding this comment.
Do you actually need these properties at all?
There was a problem hiding this comment.
I copied them from the PyKey<T> specialization: that was being used string before I added this one, so I would assume I should keep it consistent with the new specialization.
| return py::make_tuple(self.getName(), self.getDoc(), self.getUnits()); | ||
| }, | ||
| [](py::tuple t) { | ||
| if (t.size() != 3) throw std::runtime_error("Invalid number of parameters when unpickling!"); |
tests/test_schema.py
Outdated
| schema.addField("string", type="String", doc="A string field", size=42) | ||
| schema.addField("variable_string", type="String", doc="A variable-length string field", size=0) | ||
| schema.addField("array", type="ArrayF", doc="An array field", size=10) | ||
| schema.addField("variable_array", type="ArrayF", doc="A variable-length array field", size=0) |
There was a problem hiding this comment.
Lots of code duplication; consider factoring the two schema definitions into their own function (makeTestSchema? addCommonTestFields?).
parejkoj
force-pushed
the
tickets/DM-20019
branch
from
a4aef40
to
bb71b6b
Compare
Improve unpickling error messages
parejkoj
force-pushed
the
tickets/DM-20019
branch
from
bb71b6b
to
6ee9f7a
Compare
No description provided.