Skip to content

Conversation

@guitargeek
Copy link
Contributor

This is to test the impact of such an interface change.

This is to test the impact of such an interface change.
@github-actions
Copy link

Test Results

    20 files      20 suites   3d 8h 2m 9s ⏱️
 3 752 tests  3 752 ✅ 0 💤 0 ❌
74 243 runs  74 243 ✅ 0 💤 0 ❌

Results for commit a3aeace.

@vepadulano
Copy link
Member

I am slightly uneasy with doing this interface change systematically: this would affect many pillars of the project (IO, TTree, RNTuple, RDF, RooFit). Is there any chance we could add a new overload instead of replcaing, or would that become ambiguous?

@guitargeek
Copy link
Contributor Author

guitargeek commented Nov 26, 2025

IMHO, adding new overloads for the sake of Python unnecessarily blows up the C++ interface.

But std::filesystem::path is indeed not perfect. I can already see some problems:

  • <filesystem> might be a heavy import
  • TString doesn't implicitly convert to std::filesystem::path. Only to string or string_view.
  • You might have to link the filesystem library for some compilers

Actually, I would prefer to have our own adapter type, like RFilePath, which can be implicitly constructed from all string types that we supported before. And then in the Pythonizations, we can register the custom converter to RFilePath from a pathlib.Path.

Would your worry also apply to this approach?

@vepadulano
Copy link
Member

Would your worry also apply to this approach?

Yes, for different reasons. While it's a bit better than filesystem in terms of dependency (as you say some compilers still need to link explicitly against that library), I'm still not convinced that we want to change already existing interfaces in large amounts across different components of the project. Not saying no, but what I'm saying is that this needs a thorough analysis of how many places we would want to touch, and if it makes sense at all. The other disadvantage of this approach is that we're introducing our own class for something that admittedly already exists, also the naming should be discussed (I remember RFilePath was already discussed somehow in the context of RFile, we should try to disambiguate that too).

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