G-Node / nix

Neuroscience information exchange format
https://readthedocs.org/projects/nixio/
Other
66 stars 36 forks source link

Generic entity access methods for Block #679

Closed gicmo closed 7 years ago

gicmo commented 7 years ago

Instead of having specialised {has,get,count,remove}XYZ methods defined on IBlock, we now have generic methods; this removes code duplication in the backend. This is also step one to generalise the bulk replacement of entities. Additionally a new Identity class is used for these methods that should solve issue #673. The current PR does it for Block and the hdf5 backend. Will break the filesystem backend.

gicmo commented 7 years ago

Maybe you guys could have a brief glance over it, just in case we are doing something obviously stupid.

cgars commented 7 years ago

Will break the filesystem backend.

may be i am mad but does it?

cgars commented 7 years ago

A i see the fs stuff came late to the party... Sry for the buzz

gicmo commented 7 years ago

Well, as long as the front end methods keep name_or_id we will have the ambiguity. I wasn't aware that we wanna get rid of them (they are quite convenient for API users I guess). And you are right currently the behavior of the code is to work with happily with incomplete Identitys, i.e. if we have incomplete information (i.e. name_or_id) we set only one of the fields. Maybe we should change the frontend objects to have functions with dedicated id and name parameters but that would be a bigger API change. The current (already quite big) changeset is only changing internal things; but it would certainly pave the way for that bigger API change.

achilleas-k commented 7 years ago

That's fair. I agree on not changing the API for now. I was mostly concerned about the behaviour of the Identity class. The API and internals could be the same with the small difference that the Identity is always a name and ID, so when a method requires creating an Identity object but only has access to a name, it would be the responsibility of the method to fetch the ID first and then create an Identity, rather than creating an Identity with name only and using that to fetch the ID.

It's not that important though, but something to keep in mind further down the line maybe.

APPROVED

jgrewe commented 7 years ago

From the user's perspective the name is the most convenient and natural thing to do. In relacs we often do not have the concrete Entity at hand (and hence do not have the id) but there is a rule how unique names are generated and I can look if, e.g. a Tag, with such a name already exists. Requiring name and id would mean to have access to the actual entity, and then I probably do not need the method anymore. I see your point but do not see, how we can avoid this entirely without a loss in usability.

achilleas-k commented 7 years ago

But the Identity class isn't user-facing, is it?

Let me clarify my position: I'm mostly thinking about future development falling into the same trap of identifying objects by name or id, but not both. I saw the introduction of this new class and methods as a way to enforce using name and id (internally) for identification of objects. Allowing Identity objects to possess only one of the two doesn't enforce this. My thinking is that it would be nice if we (not the user) can be certain that any Identity instance refers to an object and is 100% unambiguous, because it is guaranteed to have a name and an id.

gicmo commented 7 years ago

I guess if we had the resolveNameOrId(const std::string &name_or_id) and return an Identity we could use in all user facing methods that get name_or_id and then internally we would only have unambiguous Identitys. That is what @achilleas-k wants, right?

achilleas-k commented 7 years ago

Exactly