Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Link collection to the tests #283

Merged
merged 5 commits into from
Feb 11, 2025
Merged

Add a Link collection to the tests #283

merged 5 commits into from
Feb 11, 2025

Conversation

jmcarcell
Copy link
Member

we didn't have any, and things like #282 wouldn't happen because that would have not compiled.

BEGINRELEASENOTES

  • Add a Link collection to the tests

ENDRELEASENOTES

Comment on lines 120 to 122
std::stringstream error;
error << "Wrong data in links collection, link" << j << " expected " << recos[0] << ", " << particles[j]
<< " got " << links[j].getFrom() << ", " << links[j].getTo();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't need to happen in this PR (or maybe not at all?), but we are starting to get a wild mix of fmt::format and std::stringstream for formatting error messages. I think we should settle for one for Key4hep and then try to use that consistently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's go for std::format once that Ubuntu 22 is dropped which will be soon since Gaudi hasn't been supporting GCC 11 for a while and it always has to be patched.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can go for fmt for now in any case. Gaudi uses that internally quite heavily, in that case we wouldn't need to rely on compiler support as much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we change it once to change it later to std::format, and if we were to follow the systems that are supported by Gaudi we would already have compiler support for it (they only build for GCC >= 13 and Clang >= 16, same as the LCG stacks) so I would perefer to keep the std::stringstreams and do the changes at once in the not so distant future.

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor formatting thing, otherwise looks good to me.

Co-authored-by: Thomas Madlener <[email protected]>
@jmcarcell jmcarcell enabled auto-merge (squash) February 11, 2025 08:15
@jmcarcell jmcarcell merged commit f55aff7 into main Feb 11, 2025
7 of 9 checks passed
@jmcarcell jmcarcell deleted the add-links-to-tests branch February 11, 2025 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants