From 24df6781a1f5120ec18e65edb27dcd3b5df3337d Mon Sep 17 00:00:00 2001 From: Matthew Brett Date: Sat, 19 Sep 2015 15:52:44 -0700 Subject: [PATCH 1/3] RF: remove imageopener decorator 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. --- nibabel/freesurfer/mghformat.py | 6 ++--- nibabel/openers.py | 40 ++++----------------------------- nibabel/tests/test_openers.py | 5 +---- 3 files changed, 8 insertions(+), 43 deletions(-) diff --git a/nibabel/freesurfer/mghformat.py b/nibabel/freesurfer/mghformat.py index f63875b2c6..1091e256a9 100644 --- a/nibabel/freesurfer/mghformat.py +++ b/nibabel/freesurfer/mghformat.py @@ -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 = () diff --git a/nibabel/openers.py b/nibabel/openers.py index 26bbb09d7b..fe4e09fc22 100644 --- a/nibabel/openers.py +++ b/nibabel/openers.py @@ -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 diff --git a/nibabel/tests/test_openers.py b/nibabel/tests/test_openers.py index 08f9730ace..6f859b5720 100644 --- a/nibabel/tests/test_openers.py +++ b/nibabel/tests/test_openers.py @@ -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'): From 4ff4bb21f171396887b05bd4c96373f9730e6be7 Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Wed, 21 Oct 2015 11:58:43 -0400 Subject: [PATCH 2/3] valid_exts is no longer used --- nibabel/tests/test_openers.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/nibabel/tests/test_openers.py b/nibabel/tests/test_openers.py index 6f859b5720..2900a0437e 100644 --- a/nibabel/tests/test_openers.py +++ b/nibabel/tests/test_openers.py @@ -93,8 +93,6 @@ def test_BinOpener(): class TestImageOpener: - valid_exts = () - def setUp(self): self.compress_ext_map = ImageOpener.compress_ext_map.copy() From 212076f59c1ad3561c956898adf5b017c13d34ea Mon Sep 17 00:00:00 2001 From: "Christopher J. Markiewicz" Date: Wed, 21 Oct 2015 12:19:47 -0400 Subject: [PATCH 3/3] DOC: Add usage to ImageOpener docstring --- nibabel/openers.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/nibabel/openers.py b/nibabel/openers.py index fe4e09fc22..1969766a55 100644 --- a/nibabel/openers.py +++ b/nibabel/openers.py @@ -153,6 +153,18 @@ class ImageOpener(Opener): A trivial sub-class of opener to which image classes can add extra extensions with custom openers, such as compressed openers. + + To add an extension, add a line to the class definition (not __init__): + + ImageOpener.compress_ext_map[ext] = func_def + + ``ext`` is a file extension beginning with '.' and should be included in + the image class's ``valid_exts`` tuple. + + ``func_def`` is 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`. """ # Add new extensions to this dictionary compress_ext_map = Opener.compress_ext_map.copy()