Skip to content

Conversation

@rdvincent
Copy link

Implement #272. This is still something of a work in progress, but it has reached the point where I believe it will read most MINC 1.0 and MINC 2.0 files correctly. Ideally there should be more testing, enhancement, and documentation before any actual release incorporating this change. But I would like to get comments and review started.

The HDF5 code is large and hairy, which reflects the size and complexity of the specification. HDF5 files incorporate two different classes of objects, and the different classes can coexist in one file. The format is full of interdependencies between different structures, making abstraction a challenge.

In comparison, the NetCDF "classic" format is simple, so the code is much easier to write and read.

dv_offset = new_off;
}
switch (typ) {
case type_enum.INT8:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick. Can you properly indent the case statement ? Or you indent like that due to jshint ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally follow the common convention of keeping the "case" clauses at the same indentation as the "switch" statement. The brainbrowser codebase is inconsistent here - some of the existing code indents cases, some does not.

@natacha-beck
Copy link
Contributor

@rdvincent I read all your code. Make some little comments. Nice job, it will be really useful.
A few general comments:

  • I'm little be lost with code comments, lot of yours function comments does not follow docular rules for comment. I'm not sure which one will have documentation or not, maybe you can take a second look.
  • I saw some console.log without if (debug) I don't know if it is what you want.

@rdvincent
Copy link
Author

@natacha-beck I will try to formalize some of the comments to conform to docular. I don't really know the docular standard, but I'll try to follow it where appropriate. Up until now I was mostly just trying to leave comments for myself.

@natacha-beck natacha-beck merged commit c38c0e0 into aces:master Jun 14, 2016
@rdvincent rdvincent deleted the rdv-hdf5 branch June 15, 2016 16:51
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

Successfully merging this pull request may close these issues.

2 participants