Skip to content
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

Type Definition for HDF5 module #58

Closed
NINI1988 opened this issue May 9, 2018 · 24 comments
Closed

Type Definition for HDF5 module #58

NINI1988 opened this issue May 9, 2018 · 24 comments

Comments

@NINI1988
Copy link
Contributor

NINI1988 commented May 9, 2018

Hello and thanks for provinding this great module.

Is there a plan to include type definitions for typescript?
Otherwise I would like to implement and distribute it on https://github.com/DefinitelyTyped/DefinitelyTyped.

@rimmartin
Copy link
Collaborator

rimmartin commented May 15, 2018

I'm not familiar with what this is:-)

Would it be best to be a part of this code or separate in https://github.com/DefinitelyTyped/DefinitelyTyped?

I'm guessing this adds an interface for typescript calls?

@NINI1988
Copy link
Contributor Author

I guess its better to be part of this module, because of version mismatches.
I'm currently writing the type definition and could make another pull request in the next few days.

It's only an extra definition file of the javascript interface. So it doesn't change existing code.

@rimmartin
Copy link
Collaborator

Right. And this adds the potential of typescript users liking this project?

Is there anything you need changed to make it nice?

@NINI1988
Copy link
Contributor Author

Yes, absolutly.

There is one thing.
The properties of the buffer like rank, row, and type would be better to be placed in the options argument. Then these properties would be set to required in typescript, so no one can miss to set these properties. Then the buffer is a "native" object without modified properties. But this could be a breaking change for existing projects? Or maybe enable both ways to define these properties?

@rimmartin
Copy link
Collaborator

Oh I've been wanting to move to that style with the options! Then properties are clear of any reserved names. And it would fit javascrpt/JSON coding styles.

Been thinking to make the legacy of reserved rank row, and type properties only work if no options are defined.

@rimmartin
Copy link
Collaborator

In other words this can be made seamless for prior users without breaking code they already have

@rimmartin
Copy link
Collaborator

rimmartin commented May 16, 2018

I'll work at changing to the options args for rank rows type this evening. You morning seems earlier than mine:-)

@NINI1988
Copy link
Contributor Author

Legacy must also be available when someone defines chunkSize or compression in options but use the old buffer options.

@rimmartin
Copy link
Collaborator

Yep. Are you fine with getting options back as done in https://github.com/HDF-NI/hdf5.node/blob/master/test/test_h5lt.js#L136 ?

const readBuffer=h5lt.readDataset(group.id, 'Quadruple Rank', function(options) {
                    JSON.stringify(options).should.equal('{"rank":4,"endian":0}')
                });

@NINI1988
Copy link
Contributor Author

Yes, that's fine.

@rimmartin
Copy link
Collaborator

All you're doing is really strengthening this project! Thank you

@NINI1988
Copy link
Contributor Author

Thank you.

But you are one who created this great module and housekeeping the code.
Thanks too.

@NINI1988
Copy link
Contributor Author

Maybe when we now changing the options, dims should be an array like: dims[2, 4, 5, 6] and maxDims[-1, 4,5,6];
This could make the code a little bit cleaner.
Then maybe remove the legacy properties and make a new version 0.4.0, so no user get the new spec without action.
And add a hint to the readme about the new function call.

@rimmartin
Copy link
Collaborator

rimmartin commented May 17, 2018

Yes, a release at 0.3.4 so that bug fixes are available to anyone with legacy style, tag it so a branch can be made if necessary; then make it all with the new spec.

Also I'm seeing a lot of compile warnings now with nodejs v10.1.x which I want to investigate.

Docs will get an update including the tutorials. I got some of the way thru moving to options last night but first found some other bugs.

@rimmartin
Copy link
Collaborator

Should have it testable later today with {rank: 2, rows: 10, columns: 10 ....} style

sutoiku most likely is working on nodejs 10.1.x changes in the v8

@rimmartin
Copy link
Collaborator

What should be documented for typescript developers?

@NINI1988
Copy link
Contributor Author

Maybe add an example

import { H5Type, Access, Interlace, H5OType, HLType, H5SType } from 'hdf5/lib/globals';
import { hdf5, h5lt, Hdf5Buffer } from 'hdf5';

const file: hdf5.File = new hdf5.File('test2.h5', Access.ACC_TRUNC);
const buffer: Hdf5Buffer = Buffer.alloc(8 * 10 * 8);
buffer.rank = 2;
buffer.rows = 8;
buffer.columns = 10;
buffer.type = H5Type.H5T_NATIVE_DOUBLE;

for (let j: number = 0; j < buffer.columns; j++) {
    for (let i: number = 0; i < buffer.rows; i++) {
        if (j < (buffer.columns / 2)) {
            buffer.writeDoubleLE(1.0, 8 * (i * buffer.columns + j));
        }
        else {
            buffer.writeDoubleLE(2.0, 8 * (i * buffer.columns + j));
        }
    }
}
h5lt.makeDataset(file.id, 'Waldo', buffer, { maxRows: H5SType.H5S_UNLIMITED, chunkSize: 10, compression: 7 });
file.close();

and add a hint that not all functions are currently described in typescript.

@rimmartin
Copy link
Collaborator

rimmartin commented May 22, 2018

Ah good

May I use your full name in credits?

@NINI1988
Copy link
Contributor Author

Yes and thanks.

@rimmartin
Copy link
Collaborator

btw the current waldo test and tutorial has been changed to use the options way instead of the reserved properties

You want docs on the optional maxRows: H5SType.H5S_UNLIMITED capability right? Will add.

I was asking do typescript developers know what to do? How to make developers become aware of your new dts files? I'll learn a bit to help maintain

@NINI1988
Copy link
Contributor Author

Great.

Should we add directly the maxDims: [-1, 100] option?

I develop in vs code, and the d.ts files are automatically recognize, so the original Javascript functions get a description and parameters get description.

@NINI1988
Copy link
Contributor Author

The d.ts files are like an overlay about your existing Javascript functions. So the Javascript and dts files must match, otherwise there will be Javascript errors like: undefined is not a function.

@rimmartin
Copy link
Collaborator

maxDims as an array? Might be what users gravitate towards. Will look into it

@NINI1988
Copy link
Contributor Author

That's the same as you mentioned in #42 higher rank datasets.

@NINI1988 NINI1988 closed this as completed Jun 2, 2018
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

2 participants