AIDASoft / podio

PODIO
GNU General Public License v3.0
23 stars 57 forks source link

Add a header file including all the collections #606

Closed jmcarcell closed 2 months ago

jmcarcell commented 3 months ago

BEGINRELEASENOTES

ENDRELEASENOTES

The usage would be

#include "edm4hep/edm4hep.h"

and all the collections will be included.

I find from time to time I need to do something with all the collections and after having a look at other options (hardcoding the includes means they always have to be changed; having some CMake magic in EDM4hep to generate this file is an option but it also can be done at the podio level) I think this is the cleanest one and other people can maybe also use it. In practice, if an algorithm does something with the collections and a new one is added, even if the include doesn't change because it comes from this file other code that does something with collections will have to be changed.

jmcarcell commented 3 months ago

Looks reasonable to me. I am not sure if AllCollections.h is the best name, but I don't have a better one either. Maybe something that conveys better that this is the complete datamodel?

You also need to add the new template here to trigger re-generation in case the template changes: master/python/templates/CMakeLists.txt

I changed it to, for example for EDM4hep, edm4hep/Alledm4hepCollections.h

hegner commented 3 months ago

Why don't we just call it edm4hep/edm4hep.h ? Because it is just edm4hep we are including with that. Or datamodel/datamodel.h ?

m-fila commented 3 months ago

I think I prefer edm4hep/AllCollections.h over edm4hep/Alledm4hepCollections.h but just edm4hep/Collections.h reads even better for me Name of this header technically becomes reserved for componenents/datatatypes/interfaces (for instance 'DatamodelDefinition' is already reserved as using it as a name for anything would collide with DatamodelDefinition.h). It's super formal and pedantic but a list of reserved keywords could be documented and potentially validated

Maybe something that conveys better that this is the complete datamodel?

edm4hep/Datamodel.hor edm4hep/edm4hep.h? Wouldn't the components not used in any datatype be missing though?

hegner commented 3 months ago

edm4hep/Datamodel.hor edm4hep/edm4hep.h? Wouldn't the components not used in any datatype be missing though?

the missing components won't be needed in any case. Maybe for downstream code that would then include explicitly anyhow.

tmadlener commented 2 months ago

As decided during the EDM4hep meeting on Jun 4 the name of the header should be datamodel/datamodel.h.

tmadlener commented 2 months ago

I changed the name of the generated header and also changed the generator logic to ensure that it is installed, as I don't see a reason not to install it.

jmcarcell commented 2 months ago

Looks good