Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIfTI Extension redesign #1335

Closed
effigies opened this issue Jul 3, 2024 · 0 comments · Fixed by #1336
Closed

NIfTI Extension redesign #1335

effigies opened this issue Jul 3, 2024 · 0 comments · Fixed by #1336

Comments

@effigies
Copy link
Member

effigies commented Jul 3, 2024

Right now the definition of Nifti1Extension is (simplified):

class Nifti1Extension:
    def __init__(self, code, content):
        self.code = code
        self._content = self._unmangle(content)

    def get_content():
        return self._content

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.

class Nifti1Extension:
    @property
    def content(self) -> bytes:
        return self._content

    @property
    def text(self) -> str:
        return self._content.decode(getattr(self, 'encoding', 'utf-8'))

    def json(self, **kwargs) -> Any:
        return json.loads(self._content.decode(), **kwargs)

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:

class NiftiExtension(ty.Generic[T]):
    _object: T

    def _unmangle(self, contents: bytes) -> T:
        ...

    def _mangle(self, obj: T) -> bytes:
        ...

    def get_content(self) -> T:
        return self._unmangle(self._content)

    # Above stuff...

Interested in feedback. Will open a PR with some initial work once I have something that typechecks and tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant