Add method to retrieve path from Node or Dataset (issue #314). Add method "rename"

[kerim371]

Sometimes you need to retrieve the name and the path of the node (File/Group) or dataset and attribute.
For File/Group/Dataset hdf5 uses function: ssize_t H5Iget_name( hid_t obj_id, char *name, size_t size ) and this returnes path to these objects
For attribute it uses another function: ssize_t H5Aget_name(hid_t attr_id, size_t buf_size, char *buf )

To check the implemented function you can run:

#include <H5File.hpp>
#include <H5Group.hpp>
#include <H5DataSet.hpp>
#include <H5DataSpace.hpp>
#include <H5Attribute.hpp>

using namespace HighFive;

int main(int argc, char *argv[])
{
    /* We are going to create group in root directory then add
     * dataset to this group and attach attribute to the dataset.
     * Then we are trying to get path to the root, group dataset
     * and the name of the dataset.
     * Secondly we will move dataset with attached attribute to
     * some destination path. Every '/' sign means that a group
     * will be created there (it is done automatically).
     * When we have moved the dataset we can check if dataset
     * object is still valid? We do this by creating second
     * attribute. There is a tricky part at the end when we
     * move Group with its dataset */

    // Create a new file using the default property lists.
    HighFive::File file("names.h5", File::ReadWrite | File::Create | File::Truncate);

    // Create a group
    HighFive::Group group = file.createGroup("group");

    // Create a dummy dataset of one single integer
    DataSet dataset = group.createDataSet("data", DataSpace(1), AtomicType<int>());
    dataset.write(100);

    // Now let's add a attribute to this dataset
    std::string string_list("very important Dataset !");
    Attribute attribute = dataset.createAttribute<std::string>("attribute", DataSpace::From(string_list));
    attribute.write(string_list);

    // Get path and names
    std::cout << "root path: \t" << file.getPath() << std::endl;
    std::cout << "group path: \t" << group.getPath() << std::endl;
    std::cout << "dataset path: \t" << dataset.getPath() << std::endl;
    std::cout << "attribute name: \t" << attribute.getName() << std::endl;
    std::cout << std::endl;

    // move dataset with its attribute to another destination path
    group.moveObject(file, "data", "/NewGroup/SubGroup/movedData");

    // as you can see to reach destination path new groups were created as well
    std::cout << "dataset new path: \t" << dataset.getPath() << std::endl;
    std::cout << std::endl;

    // we can still use moved dataset
    // let's create new attribute
    Attribute attributeNew = dataset.createAttribute<std::string>("attributeNew", DataSpace::From(string_list));
    attribute.write(string_list);
    std::cout << "attribute new name: \t" << attributeNew.getName() << std::endl;
    std::cout << std::endl;

    // move the folder with its content to other place
    file.moveObject("/NewGroup/SubGroup", "/FinalDestination");

    // here is the important moment. Old 'dataset' variable tells us
    // that dataset directory wasn't changed
    std::cout << "dataset new path wasn't changed: \t" << dataset.getPath() << std::endl;
    std::cout << std::endl;

    // but actually it was moved we just need to update variable
    std::cout << "actually it was moved we just need to update it: \t" << file.getDataSet("/FinalDestination/movedData").getPath() << std::endl;
    std::cout << std::endl;

    /* The conclusion: if you move a Group always update the varibles
     * to its content :) */

    file.flush();
}

[kerim371]

Also I added the method moveObject(...) wich is based on herr_t H5Lmove( hid_t src_loc_id, const char *src_name, hid_t dest_loc_id, const char *dest_name, hid_t lcpl_id, hid_t lapl_id ) hdf5 library function
Only File and Group has this method.

[tdegeus]

A first question is : could you add some tests in HighFive/tests/unit. That would help verification as well as grasping the functionality.

[kerim371]

@tdegeus just added here

[tdegeus]

Thanks! Do you think the tests could be added to tests_high_five_base.cpp? Then they would be immediately included in the continuous integration.

[kerim371]

@tdegeus actually I don't know how test should look like
I just made simple example how to implement written function
If my test is badly written probably I could change it. At least rewrite the comments to make it more clear and more readable probably

[matz-e]

Thanks, seems like nice functionality (that I think I wanted to have at some point). Some nudges here…

[matz-e]

Seems to me that this code repeats itself several times, save for this little function. Could probably be deduplicated? Also the naming could be shortened to getName and getPath without repeating the class name in the method name…

[kerim371]

Hi,
I absolutely agree with your proposition about changing the names of the methods. So now they are called getName and getPath

Seems to me that this code repeats itself several times, save for this little function. Could probably be deduplicated?

I still don't fully understand how this library works because I have huge number of errors inside the project :)
I guess this is beacause one namespace is divided in many .hpp files and when I compile the code it works because those files are combined I think.
But editing the code is difficult in such conditions. In this case creating new small functions that decrease the number of code lines would make the code less readable for me and probably for other developpers who probably would read this code.
So if you don't mind I would leave it as is.

[tdegeus]

@KerimMatlab Nice features you introduced!

I do have some suggestions to improve. If you agree I'll directly push them to the pull request.

My comments are that:

• getPath and getName could be moved to Object to potentially widen their scope. Both Attribute and DataSet are derived from it.
• I would keep the API file.moveObject("/old/absolute/path", "/new/absolute/path") and file.moveObject(group, "old/relative/path", "new/relative/path").
• I don't see a use for getPath and getName in NodeTraits, but maybe I'm overlooking something?

[kerim371]

Why there are three declined status (Review required, All checks have failed, Merging is blocked)?
It is my first time adding pull request. Can't understand what should I do

[tdegeus]

The "1 failing check" is because the continuous integration (CI) is firing an error. In this project all compiler warning do this. You can click on details to check out the reason. Furthermore this project requires at least one reviewer to explicitly approve before the PR can be merged by somebody with the proper rights.

[tdegeus]

Let me know if you're ok if I push some suggestions (we can always revert some of them)

[matz-e]

My point of view: if you want that moveObj would be rename (or some other) just say 'please rename moveObj() to rename'.

I wasn't sure if the naming should be changed. You're already passing in an object, so in my opinion, the Object in the name is a bit superfluous. But ultimately, I'm not the only that is responsible for this repository. Hence me not flat out requesting this to be changed. Maybe @ohm314 could give his opinion if highfive::Group::moveObject could have the Object dropped?

I'll have a look deduplicating the other code…

[kerim371]

@matz-e I just replaced moveObj() by rename().
Thank you, now the code looks cleaner and I accepted your commit. As I understood you used lambda expression (which I don't know yet) but there was a place where you forgot to replace the code. So I did it by myself and as I don't know lambda expressions please look at the commit have I done it correctly?

[tdegeus]

You did it almost correctly! The only thing that needed improvement was that you did not allow for a variably length, by hand-specifying it. I corrected it, along with another issue that appeared for a reason unclear to me. Anyway since it was a bug it is good that it is now fixed.

[tdegeus]

Very nice cleanup @KerimMatlab and @matz-e !

[matz-e]

Very nice cleanup @KerimMatlab and @matz-e !

I was just giving it another look to approve; One last thing (sorry!): could we have a test for Attribute::getName, too? Then I'm ready to approve myself

[matz-e]

Thank you very much, @KerimMatlab and @tdegeus! Apologies that this dragged on for a while.

[matz-e]

Merging after no additional dissenting feedback received.

[matz-e]

Thank you again for your contributions!