You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This means that the return type of get_content() depends on the subclass, violating the Liskov substitution principle and generally making it difficult to reason about the value of extension.get_content() without either inspecting the type of extension or extension.get_content().
In #1327 I've found that there are several places in our tests where we've assumed that get_content() returns bytes, so it is likely that downstream users will also be broken. This seems like an opportunity to redesign.
Instead we could consider following the lead of requests.Response/httpx.Response and provide a .content property that is always bytes, a .text property that is always str (raising an error if decoding fails) and a .json() method that decodes the JSON object, which can fail if the contents are not valid JSON.
This could sit alongside the old, variable-type get_content().
I haven't fully thought through what we want to do for extensions that need to expose complex data types, like pydicom Datasets or Cifti2Headers. I'll experiment a little with this with typing to make sure that what I do fits in with typed Python. Probably something like:
Right now the definition of
Nifti1Extension
is (simplified):This means that the return type of
get_content()
depends on the subclass, violating the Liskov substitution principle and generally making it difficult to reason about the value ofextension.get_content()
without either inspecting the type ofextension
orextension.get_content()
.In #1327 I've found that there are several places in our tests where we've assumed that
get_content()
returnsbytes
, so it is likely that downstream users will also be broken. This seems like an opportunity to redesign.Instead we could consider following the lead of
requests.Response
/httpx.Response
and provide a.content
property that is alwaysbytes
, a.text
property that is alwaysstr
(raising an error if decoding fails) and a.json()
method that decodes the JSON object, which can fail if the contents are not valid JSON.This could sit alongside the old, variable-type
get_content()
.I haven't fully thought through what we want to do for extensions that need to expose complex data types, like pydicom
Dataset
s orCifti2Header
s. I'll experiment a little with this with typing to make sure that what I do fits in with typed Python. Probably something like:Interested in feedback. Will open a PR with some initial work once I have something that typechecks and tests.
The text was updated successfully, but these errors were encountered: