RF: remove imageopener decorator #361
Conversation
|
Looks OK to me. My only concern is exposing What about just calling |
|
I don't feel very strongly either way, so I think it's good that you popped in. That does seem to be a decent compromise between either To my mind a function makes sense if you want a constraint. For instance, we currently disallow setting an entry twice, which this PR does not preserve. If that's worth keeping, then I'd say we go ahead with a lightweight wrapper around |
|
I think we aren't exposing much, just that the ImageOpener has a dict
interface...
|
|
@matthew-brett After further review, I think you're right. I'm still not used to overriding operators in Python, it seems! LGTM thanks! |
|
Will note again that we're losing the non-overwrite constraint, but if you're both okay with that, I'm okay with that. |
|
Fine with me! |
|
Removed an unused line and adapted the usage docs from the decorator to the class itself. |
effigies
force-pushed
the
remove-imageopener-decorator
branch
from
68a035a to
212076f
Compare
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.
| ``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`. |
|
Re-reviewed, still LGTM! |
RF: remove ImageOpener decorator Image classes should directly update the ImageOpener.compress_ext_map dictionary.
RF: remove ImageOpener decorator Image classes should directly update the ImageOpener.compress_ext_map dictionary.
Popping this up from @matthew-brett's fork off #329.
Pros for the decorator:
valid_extsand specifying how it should be opened happen inthe same place (decorator at top of class);
class.
Cons:
call (so the decorator returns a decorator);
decorator is at the top of the class, and adds later to
valid_exts,whereas the apparent definition of
valid_extsis after the docstring;the
valid_extsattribute);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_extsdefinition;the dictionary a property or another object type, and changing the
underlying storage;