-
-
Notifications
You must be signed in to change notification settings - Fork 208
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
Update tinyformat #791
Update tinyformat #791
Conversation
I'll probably merge Kevin's PR next, and if "just merge" this we get nasty graph. Shall we bother with rebasing or should I just squash this in? Undecided so far if it needs rev.dep run by itself. Maybe ... |
I can rebase if it helps. Still need to update |
I like cleanly separated paths when I do Myself, I mostly get lost with rebase. I think I did it right just once or twice. So squash would be an easy out. But if you can "graft" your few commits behind Kevin's (once I merge them in an hour or two to three) then I'd be happy in a typically nerdy type-A way that does really matter :) |
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.
The #include<cassert>
was commented out on the local modification. I can't remember if that was because CRAN can pick this include up on checks even if it isn't used.
Answer seems to be no:
* checking compiled code ... OK
//------------------------------------------------------------------------------ | ||
// Implementation details. | ||
#include <algorithm> | ||
#include <cassert> |
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.
Could the #include<cassert>
be moved to the #ifndef TINYFORMAT_ASSERT
in upstream tidyformat?
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.
Maybe: c42f/tinyformat#49
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.
@krlmlr awesome!
Merged other PR, wrapped this up a local package and started a new reverse depends check. Mine are still serial (working on that ...) so it'll be tomorrow. Plenty of time for you to rebase if you feel like it. |
Fixes #786.