-
Notifications
You must be signed in to change notification settings - Fork 73
Give a friendlier failure when lacking knitr #1267
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
Conversation
This is how benchmark results would change (along with a 95% confidence interval in relative change) if b7ad523 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
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.
LGTM, thanks.
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2f971a8 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
Just weird that this seems to slow down things… |
I triggered touchstone again to see if the problem persist. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 2f971a8 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
Hmm... I think the speed test might actually be correct. Can we do the test for {knitr} more high level once, instead of every time we call |
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.
Sorry I just realised we should not merge this PR as is.
Also, @MichaelChirico, it would be great to fix this in the next 5 days, as I am planning to submit a new version of {styler} to CRAN. |
I'm not too familiar with package internals so I spent only a few minutes trying to pin down where is the right place to move the check, I guess this is good because it avoids the iterated calls here: Line 121 in fe6ae64
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 377d573 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation. |
Much better, thanks @MichaelChirico 🥳. +2% for the |
Closes #1266