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

[ntuple] Merger: don't abort if a file doesn't contain a RNTuple #17609

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

silverweed
Copy link
Contributor

@silverweed silverweed commented Feb 4, 2025

The file without the RNTuple will simply be skipped. This mirrors the merge behavior of other ROOT objects.
Note that if the user gave the -ff or -fk options ("use first source compression") the compression will be taken from the first RNTuple found.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

Copy link

github-actions bot commented Feb 4, 2025

Test Results

    17 files      17 suites   4d 1h 26m 40s ⏱️
 2 690 tests  2 687 ✅ 0 💤 3 ❌
44 118 runs  44 115 ✅ 0 💤 3 ❌

For more details on these failures, see this check.

Results for commit 63b95b8.

♻️ This comment has been updated with latest results.

@hahnjo
Copy link
Member

hahnjo commented Feb 5, 2025

In principle LGTM, we probably want a test for this, in roottest? Also again a very picky comment (sorry): our own CONTRIBUTING.md says we should use present imperative mood, so the commit message should be "Merger: skip files without RNTuple".

@silverweed
Copy link
Contributor Author

our own CONTRIBUTING.md says we should use present imperative mood, so the commit message should be "Merger: skip files without RNTuple".

But "don't abort" is present imperative; am I missing something?

@hahnjo
Copy link
Member

hahnjo commented Feb 5, 2025

But "don't abort" is present imperative

You are right. I thought we also prefer positive message instead negations (well, at least I do), but then please disregard.

Copy link
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

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

LGTM; please add a test, I'd suggest a unit test in ntuple_merger using TFileMerger.

This mirrors the merge behavior of other ROOT objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants