Skip to content

parrec/minc1 don't subclass ArrayProxy #842

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

Open
rmarkello opened this issue Nov 18, 2019 · 2 comments
Open

parrec/minc1 don't subclass ArrayProxy #842

rmarkello opened this issue Nov 18, 2019 · 2 comments

Comments

@rmarkello
Copy link
Contributor

While looking at #833 I noticed that neither PARRECArrayProxy nor MincImageArrayProxy subclass ArrayProxy despite there being considerable overlap in their functionality and structure. It seems like it would be reasonable to modify both classes such that they directly inherit from ArrayProxy to reduce duplication of some methods / doc-strings and make them a bit easier to keep in-line with ArrayProxy functionality. (Though I hope there's not a whole lot more changes coming to ArrayProxy in the near future.)

To be fair, based on my understanding neither parrec nor minc1 are widely used formats anymore, so this would be a relatively low-impact change for what could be a headache of a PR. Still, figured I'd bring it up for discussion and see what people thought. I'd be happy to try my hand at this in the coming weeks, if there's interest.

@matthew-brett
Copy link
Member

I seem to remember I made a deliberate decision not to sub-class. The array proxy was always meant to be an API, so it made sense for different classes to implement the API. On the other hand it's perfectly possible that led to some code-duplication. If there is some, that would be cleaned up by a PR, it would be worth giving a shot. Maybe just do the basic stuff to see what it looks like?

@effigies
Copy link
Member

There are optional parts of the API, so perhaps it would make more sense to abstract out the mandatory parts into a(n abstract?) base class?

Some scale and some don't, so that might be a sensible branch in the hierarchy.

I suspect a large part of the duplication was added in #833, so this may only just now be starting to be worth it.

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

No branches or pull requests

3 participants