-
Notifications
You must be signed in to change notification settings - Fork 151
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
Wrapper FMT header for compatibility #615
Conversation
for special eigen formatter, which is not available in fmt9 fmtlib/fmt#3465 https://stackoverflow.com/a/73755864/21260084
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #615 +/- ##
==========================================
- Coverage 58.82% 57.77% -1.05%
==========================================
Files 91 132 +41
Lines 8623 10689 +2066
Branches 0 959 +959
==========================================
+ Hits 5072 6174 +1102
- Misses 3551 4468 +917
- Partials 0 47 +47 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle I approve. But the name of the wrapper include (fmt_p.h
) is too cryptic.
What is the meaning of _p
?
I suggest simply fmt.h
or fmt_wrapper.h
.
You introduced that suffix and it implies that the header will not be installed. |
I see. There the |
🃏
I'm aware. I prefer not to install it unless/until we need it in some very related downstream package too. |
It is easily possible that we will use fmt in another package of this repo (aside from core) in the future. |
It's not an external API for now and it takes one sed to change it if needed. 🐕 |
for special eigen formatter, which is not available in fmt9+ (found on Debian testing).
fmtlib/fmt#3465
https://stackoverflow.com/a/73755864/21260084
@rhaschke Not sure whether you are ok with the generic wrapper header as you usually argue for minimal includes. The concrete issue currently only affects a single location where an Eigen datatype is formatted. But I believe as we use libfmt all over the place (as you introduced at some point), I would also expect to be able to format Eigen datastructures everywhere without additional includes & helpers.