Make MetaDataHandle::get throw in case the value is not available and add more overloads#195
Conversation
|
It looks like this has a rather small impact on downstream code, so I would be in favor of having this as a breaking change, i.e. the |
|
I'm fine with that, k4FWCore doesn't build anyway on the latest release, so not much is going to change |
d8ee8aa to
976edef
Compare
| @@ -73,7 +65,7 @@ MetaDataHandle<T>::MetaDataHandle(const Gaudi::DataHandle& handle, const std::st | |||
| } | |||
|
|
|||
| //--------------------------------------------------------------------------- | |||
There was a problem hiding this comment.
I have added a docstring that only mentions std::optional. Should it also mention that the return type depends on the version of podio? I am not entirely sure whether that does not confusing.
There was a problem hiding this comment.
Mentioning that this depends on podio version would be good, because people can see the std::optional or T in doxygen, but they can't see that this might change.
There was a problem hiding this comment.
Not sure if doxygen actually sees through the auto here. But I have added a note in any case.
Coming to think of it, maybe it should just throw an exception in case it's not available and we don't mess with the return type?
There was a problem hiding this comment.
Add the possibility to add a default value, and if no default value is given throw the exception.
There was a problem hiding this comment.
How about still having a version returning optional e.g.get_optional? I think it's pretty common to have throwing, default and optional getters for different cases. This way we don't mess-up with the return here but still provide a way to handle missing values when an exception is too much
There was a problem hiding this comment.
get_optional is done and the others are now using that and simply changing return values if necessary. Additionally, get_optional is now also the single place where all the podio version differences are handled and a std::optional is created on the fly with the heuristic check for empty values to return an empty optional.
|
Now I have to introduce pre processor checks here and check that it works for podio @0.99 as well as once #580 is merged. I will be coming back to this a bit later (i.e. probably tomorrow). |
|
Anything still missing from here, or can we merge this? |
|
Merging this, since this is not transparent and we need to unblock AIDASoft/podio#580 |
BEGINRELEASENOTES
MetaDataHandle::getthrow an exception in case the value for the handle is not (yet) available.MetaDataHandle::get(T defaultValue)overload to get a default value and no exception in case the value is not (yet) available.MetaDataHandle::get_optional()to retrieve an optional that is engaged and holds the value if the handle already has a value available.ENDRELEASENOTES
I have for now opted to let the underlying change in podio shine through here. I think having anoptionalvalue forMetaDataHandle::getoffers the same benefits as having it on theFramein the first place, as otherwise there is no way of distinguishing between empty and unset values.