AIDASoft / podio

PODIO
GNU General Public License v3.0
24 stars 60 forks source link

Make GenericParameters return optional instead of empty defaults on non-existant keys #580

Closed tmadlener closed 4 months ago

tmadlener commented 6 months ago

BEGINRELEASENOTES

ENDRELEASENOTES

Fixes #576

The following PRs make this introduction work transparently and need to be merged first to not break the nightlies

m-fila commented 6 months ago

I think getValue is an unintuitive name for a method returning optional instead of "value". Code like parameter.GetValue<int>("foo").has_value() reads a bit awkward

tmadlener commented 6 months ago

Since we are breaking interfaces in any case I don't mind different names. That would in principle allow us to properly deprecate getValue as it is. On the other hand GenericParameters is mainly used internally, so we could probably also just use new names without deprecation.

hegner commented 5 months ago

@tmadlener - ping. What's the status?

tmadlener commented 5 months ago

Currently testing what is breaking downstream (and then fixing that if necessary)

tmadlener commented 5 months ago

I think getValue is an unintuitive name for a method returning optional instead of "value". Code like parameter.GetValue<int>("foo").has_value() reads a bit awkward

I have changed this to get as we are breaking things in any case

tmadlener commented 4 months ago

@Zehvogel a quick heads up:

  • Rename the setValue and getValue functions to just set and get. This is also a breaking change, but as far as we are aware the GenericParameters are only used internally in podio.

This bit here will require some changes to your RDataFrame code where you directly talk to the PARAMETERS branch, mainly

- df = df.Define("xsec_fb", "PARAMETERS.getValue<float>(\"crossSection\")")
+ df = df.Define("xsec_fb", "PARAMETERS.get<float>(\"crossSection\")")
Zehvogel commented 4 months ago

This bit here will require some changes to your RDataFrame code where you directly talk to the PARAMETERS branch, mainly

actually needed a

- df = df.Define("xsec_fb", "PARAMETERS.getValue<float>(\"crossSection\")")
+ df = df.Define("xsec_fb", "*PARAMETERS.get<float>(\"crossSection\")")

(or something intelligent :))

tmadlener commented 4 months ago

Ah yes, because it now also returns a std::optional. so, either operator* (as in your diff) or .value() will get you the actual value and not the optional).