search

Enet4 / nifti-rs

Rust implementation of the NIfTI-1 format
Apache License 2.0
41 stars 11 forks source link

Feature/remove esize from extension interface #102

Closed twitzelbos closed 1 year ago

twitzelbos commented 1 year ago

Removed esize from the Extension::new constructor Added more ecodes

twitzelbos commented 1 year ago

Okay gentlemen, lets give it a look now. @nilgoyette @Enet4. Thanks

twitzelbos commented 1 year ago

One thing I overlooked in the previous pull request review is asking whether NiftiEcode should be marked as non-exhaustive, so that adding new variants does not break existing code. This should only be useful if the set of common extension codes tends to expand over time (maybe through custom integrations?).

I noticed that the type is currently not used anywhere, so it seems to be primarily a simple way to provide constants for known extension codes to developers writing new nifti files. If we go in this direction, we can also provide a getter at the Extension struct (in a similar fashion as getting the intent code from the header), and maybe add #[non_exhaustive].

I added another minor recommendation inline.

Yes, I created the enum for eventual future use, but haven't yet used it (in fact the extension codes I'm writing are not on this list). I can also remove the entire enum and save it for a future date, if that helps. Thoughts?

nilgoyette commented 1 year ago

I created the enum for eventual future use, but haven't yet used it (the extension codes I'm writing are not on this list).

If it's normal and common to create our own NiftiEcode , then I think an enum shouldn't be used. Unless we know that the majority of programmers use the standard ones. This is probably hard to know.

twitzelbos commented 1 year ago

Well, the extension codes in this enum are used by the respective software tools, so if you want to interact with data from those, its a nice thing to have. But many people also use their own private ones (i.e. we do), which of course wouldn't be part of an official package like this, but its nice to know which ones are officially claimed.

twitzelbos commented 1 year ago

I have removed the enum type from this PR entirely, hopefully unblocking this. @Enet4 @nilgoyette