Error for multiple different variadic concepts with the same name #646
Conversation
4f264aa to
179bba4
Compare
2553ff4 to
0225333
Compare
There was a problem hiding this comment.
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.
| except TypeError: | ||
| pass |
There was a problem hiding this comment.
|
|
||
| tree = generate_tree_from(appdef) | ||
| collector.clear() | ||
| find_instance_name_conflicts(mapping, keys_to_remove) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I had the function somewhere else first, where the global context was not available. Removed it now.
|
The identification of valid or invalid concepts don't seem to fully work yet. If I have two similar groups with several sub-fields: all keys are removed: 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. |
Co-authored-by: Laurenz Rettig <53396064+rettigl@users.noreply.github.com>
…adic key in the template already
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
This should be fixed now. I also split up the tests to cover the different cases individually. |
|
I only checked the two cases I had mentioned here again, this works fine now. LGTM. |
As discussed in #636 and #638, we should not allow the same instance name for two unnamed groups, e.g.
NXuserandNXsample. 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.