Skip to content

Commit

Permalink
RF: remove imageopener decorator
Browse files Browse the repository at this point in the history
I was finding the decorator for ImageOpener confusing.

Pros for the decorator:

* Adding allowed `valid_exts` and specifying how it should be opened happen in
  the same place (decorator at top of class);
* Using a decorator allows change of internal storage of the ImageOpener
  class.

Cons:

* Decorators can be hard to read, especially when the decorator is a function
  call (so the decorator returns a decorator);
* It's hard to see at a glance which extensions are valid, because the
  decorator is at the top of the class, and adds later to `valid_exts`,
  whereas the apparent definition of `valid_exts` is after the docstring;
* The decorator has to know about the internals of the class (in this case,
  the `valid_exts` attribute);
* The role of the decorator is to signal that a certain extension has to be
  opened in a certain way, but it seems a bit odd that this signal also adds
  the extension to the class.  To me it seems more natural to make this signal
  a line of its own, next to the `valid_exts` definition;
* Modifying the ImageOpener dictionary directly doesn't stop us from making
  the dictionary a property or another object type, and changing the
  underlying storage;
* No decorator means less code.
  • Loading branch information
matthew-brett committed Sep 19, 2015
1 parent b0d1787 commit f4b301d
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 43 deletions.
6 changes: 3 additions & 3 deletions nibabel/freesurfer/mghformat.py
Original file line number Diff line number Diff line change
Expand Up @@ -454,13 +454,13 @@ def writeftr_to(self, fileobj):
fileobj.write(ftr_nd.tostring())


# Register .mgz extension as compressed
@ImageOpener.register_ext_from_image('.mgz', ImageOpener.gz_def)
class MGHImage(SpatialImage):
""" Class for MGH format image
"""
header_class = MGHHeader
valid_exts = ('.mgh',)
valid_exts = ('.mgh', '.mgz')
# Register that .mgz extension signals gzip compression
ImageOpener.compress_ext_map['.mgz'] = ImageOpener.gz_def
files_types = (('image', '.mgh'),)
_compressed_suffixes = ()

Expand Down
40 changes: 4 additions & 36 deletions nibabel/openers.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,42 +149,10 @@ def __exit__(self, exc_type, exc_val, exc_tb):


class ImageOpener(Opener):
""" Opener-type class passed to image classes to collect compressed extensions
""" Opener-type class to collect extra compressed extensions
This class allows itself to have image extensions added to its class
attributes, via the `register_ex_from_images`. The class can therefore
change state when image classes are defined.
A trivial sub-class of opener to which image classes can add extra
extensions with custom openers, such as compressed openers.
"""
# Add new extensions to this dictionary
compress_ext_map = Opener.compress_ext_map.copy()

@classmethod
def register_ext_from_image(opener_klass, ext, func_def):
"""Decorator for adding extension / opener_function associations.
Should be used to decorate classes. Updates ImageOpener class with
desired extension / opener association. Updates decorated class by
adding ```ext``` to ```klass.alternate_exts```.
Parameters
----------
opener_klass : decorated class
ext : file extension to associate `func_def` with.
should start with '.'
func_def : opener function/parameter tuple
Should be a `(function, (args,))` tuple, where `function` accepts
a filename as the first parameter, and `args` defines the
other arguments that `function` accepts. These arguments must
be any (unordered) subset of `mode`, `compresslevel`,
and `buffering`.
Returns
-------
opener_klass
"""
def decorate(klass):
assert ext not in opener_klass.compress_ext_map, \
"Cannot redefine extension-function mappings."
opener_klass.compress_ext_map[ext] = func_def
klass.valid_exts += (ext,)
return klass
return decorate
5 changes: 1 addition & 4 deletions nibabel/tests/test_openers.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,9 @@ def file_opener(fileish, mode):

# Add the association
n_associations = len(ImageOpener.compress_ext_map)
dec = ImageOpener.register_ext_from_image('.foo',
(file_opener, ('mode',)))
dec(self.__class__)
ImageOpener.compress_ext_map['.foo'] = (file_opener, ('mode',))
assert_equal(n_associations + 1, len(ImageOpener.compress_ext_map))
assert_true('.foo' in ImageOpener.compress_ext_map)
assert_true('.foo' in self.valid_exts)

with InTemporaryDirectory():
with ImageOpener('test.foo', 'w'):
Expand Down

0 comments on commit f4b301d

Please sign in to comment.