Add a header file including all the collections #606
Conversation
There was a problem hiding this comment.
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:
https://github.com/AIDASoft/podio/blob/master/python/templates/CMakeLists.txt
| @@ -0,0 +1,5 @@ | |||
| // AUTOMATICALLY GENERATED FILE - DO NOT EDIT | |||
|
|
|||
There was a problem hiding this comment.
Should we generate include guards here?
There was a problem hiding this comment.
Added, although it should not matter much since the headers have their own
python/podio_gen/cpp_generator.py
Outdated
| collection_files = ( | ||
| os.path.basename(f) | ||
| for f in self.generated_files | ||
| if f.endswith("Collection.h") and "Mutable" not in f | ||
| ) |
There was a problem hiding this comment.
I think we should be able to get this from self.datatypes without having to filter the generated files to find the Collection.h files?
There was a problem hiding this comment.
I changed it to use self.datamodel.datatypes
I changed it to, for example for EDM4hep, |
|
Why don't we just call it |
|
I think I prefer
|
the missing components won't be needed in any case. Maybe for downstream code that would then include explicitly anyhow. |
|
As decided during the EDM4hep meeting on Jun 4 the name of the header should be |
tmadlener
force-pushed
the
all-collections
branch
from
12214cc to
b2942e3
Compare
|
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. |
|
Looks good |
python/templates/CMakeLists.txt
Outdated
| @@ -1,6 +1,7 @@ | |||
| set(PODIO_TEMPLATES | |||
| ${CMAKE_CURRENT_LIST_DIR}/Collection.cc.jinja2 | |||
| ${CMAKE_CURRENT_LIST_DIR}/Collection.h.jinja2 | |||
| ${CMAKE_CURRENT_LIST_DIR}/AllCollections.h.jinja2 | |||
There was a problem hiding this comment.
Only nitpicking comment would be to change the name of the jinja2 template as well
BEGINRELEASENOTES
<datamodel>/<datamodel>.h.ENDRELEASENOTES
The usage would be
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.