Add warning when user sets a tag instead of a distinct geometry path

[richeldichel]

This warning informs users about a possible configuration mistake --> See #118.

[2xB]

This is a great addition! I'm just not sure if this is the correct place for this. The function in which you test the path itself calls RetrieveSpacesBySpecifier, which splits the path into a list of paths. So you effectively just test the first item of a list of paths when looking at whether the path list starts with "@". Therefore I would highly suggest to move this code to RetrieveSpacesBySpecifier or - even better - RetrieveSpacesByPath, which directly gets a single path from the split pathlist. There one has to check whether aNode is fRoot, because if it is looking for a tag in a specific space that is not the root space, I think the problem doesn't exist. Essentially one could throw an error in if (tHead.find_first_of(sTag) == 0) { if aNode is fRoot?

One could also use the variable const char KGInterface::sTag = '@' instead of hard-coding "@"?

As a minor suggestion, the warning message could also be extended to something like this to make it fully clear? "Path definition just contains a tag, which can make it ambiguous: <" << aSpecifier << ">. Please specify a distinct geometry path!" << eom;

Btw. is there a reason not to make an error out of this? Is there any plausible reason RetrieveSpacesByPath would receive just a tag? Btw. KWARN_ONCE could be used to print this warning only once, but maybe erroring out is better anyways.

[richeldichel]

The function in which you test the path itself calls RetrieveSpacesBySpecifier, which splits the path into a list of paths. So you effectively just test the first item of a list of paths when looking at whether the path list starts with "@". Therefore I would highly suggest to move this code to RetrieveSpacesBySpecifier or - even better - RetrieveSpacesByPath, which directly gets a single path from the split pathlist.

I cannot move this check to RetrieveSpacesByPath because this function is called recursively and will in the end always display this warning if there is a tag at the end of your path. That is of course not when we want to call this. tHead does not always contain the full path.

One could also use the variable const char KGInterface::sTag = '@' instead of hard-coding "@"? As a minor suggestion, the warning message could also be extended to something like this to make it fully clear? "Path definition just contains a tag, which can make it ambiguous: <" << aSpecifier << ">. Please specify a distinct geometry path!" << eom;

I agree that one could put it into RetrieveSpacesBySpecifier and I also like your suggestion regarding the check and warning message.

Btw. is there a reason not to make an error out of this? Is there any plausible reason RetrieveSpacesByPath would receive just a tag? Btw. KWARN_ONCE could be used to print this warning only once, but maybe erroring out is better anyways.

I would argue that it would break many existing configs due to the fact, that e.g. for a VTK appearance definition one would often times be lazy and only supply the tag for which the appearance should be applied. An ambiguous definition there, would not cause any harm. Therefore, a warning should suffice I'd say.

[richeldichel]

Update: I get now what you mean with your first point! That's much better and I implemented it in the latest commit