Error for multiple different variadic concepts with the same name

[lukaspie]

As discussed in #636 and #638, we should not allow the same instance name for two unnamed groups, e.g. NXuser and NXsample. We implement this here by running a check on the whole mapping table before any of the other validation starts. Note that we have to remove such keys as they will necessarily lead to HDF5 conflicts (HDF5 depends on unique names for groups and datasets).

Error message may need some improvement, wasn't too sure how to report this issue.

[RubelMozumder]

LGTM!

[rettigl]

This looks good to me, however there is at least one case this does not cover:
If I define the same concept twice, once with a variadic name, once without (for a named concept), the writer will fail, because the entry is already written:
Example:
/ENTRY/INSTRUMENT/energy_resolution/type
and
/ENTRY/INSTRUMENT/RESOLUTION[energy_resolution]/type
both defined.
I think, however, that we don't have to catch such cases, as this is clearly a broken input. I think we probably can never and don't have to make this completly failsafe.

[rettigl]

When do we arrive here?

[lukaspie]

It is from here, which is used here to remove keys where we have an invalid type for a named concept. It was introduced in #638.

Not the most elegant solution, but works for now. We should probably not even reach the TypeError here, but at least we are covered.

[rettigl]

For the functions below, you don't pass the keys_to_remove, but use them from the global context. Why is this handled differently here?

[lukaspie]

I had the function somewhere else first, where the global context was not available. Removed it now.

[rettigl]

The identification of valid or invalid concepts don't seem to fully work yet. If I have two similar groups with several sub-fields:

  "/ENTRY/USER[user]": {
    "name": ,
    "role": ",
    "affiliation": ,
    "address": ,
    "email": 
  },
  "/ENTRY/ILLEGAL[user]": {
    "name": ,
    "role": ",
    "affiliation": ,
    "address": ,
    "email": 
  },

all keys are removed:

WARNING: Instance name 'user' used for multiple different concepts: ILLEGAL, USER. The following keys are affected: /ENTRY[entry]/ILLEGAL[user]/address, /ENTRY[entry]/ILLEGAL[user]/affiliation, /ENTRY[entry]/ILLEGAL[user]/email, /ENTRY[entry]/ILLEGAL[user]/name, /ENTRY[entry]/ILLEGAL[user]/role, /ENTRY[entry]/USER[user]/address, /ENTRY[entry]/USER[user]/affiliation, /ENTRY[entry]/USER[user]/email, /ENTRY[entry]/USER[user]/name, /ENTRY[entry]/USER[user]/role.
WARNING: The key /ENTRY[entry]/ILLEGAL[user]/address will not be written.
WARNING: The key /ENTRY[entry]/ILLEGAL[user]/affiliation will not be written.
WARNING: The key /ENTRY[entry]/ILLEGAL[user]/email will not be written.
WARNING: The key /ENTRY[entry]/ILLEGAL[user]/name will not be written.
WARNING: The key /ENTRY[entry]/ILLEGAL[user]/role will not be written.
WARNING: The key /ENTRY[entry]/USER[user]/address will not be written.
WARNING: The key /ENTRY[entry]/USER[user]/affiliation will not be written.
WARNING: The key /ENTRY[entry]/USER[user]/email will not be written.
WARNING: The key /ENTRY[entry]/USER[user]/name will not be written.
WARNING: The key /ENTRY[entry]/USER[user]/role will not be written.

even though the second one is invalid.

If there is just one sub-key in the valid entry, it works as expected, which is also what the test contains. I suggest to fix this and extend the test by a second sub-key. Maybe also break the test apart into several ones that test different aspects of this functionality.

[lukaspie]

This looks good to me, however there is at least one case this does not cover: If I define the same concept twice, once with a variadic name, once without (for a named concept), the writer will fail, because the entry is already written: Example: /ENTRY/INSTRUMENT/energy_resolution/type and /ENTRY/INSTRUMENT/RESOLUTION[energy_resolution]/type both defined. I think, however, that we don't have to catch such cases, as this is clearly a broken input. I think we probably can never and don't have to make this completly failsafe.

I implemented a check (+test) for this now and if the non-concept key is valid, we remove the one with the concept. That is, in your example, we remove /ENTRY/INSTRUMENT/RESOLUTION[energy_resolution]/type and keep /ENTRY/INSTRUMENT/energy_resolution/type.

The identification of valid or invalid concepts don't seem to fully work yet. If I have two similar groups with several sub-fields [...] If there is just one sub-key in the valid entry, it works as expected, which is also what the test contains. I suggest to fix this and extend the test by a second sub-key. Maybe also break the test apart into several ones that test different aspects of this functionality.

This should be fixed now. I also split up the tests to cover the different cases individually.

[rettigl]

I only checked the two cases I had mentioned here again, this works fine now. LGTM.

[rettigl]

LGTM