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.

Sign up for GitHub

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

Jump to bottom

docs: edits to vignettes #958

Merged
merged 6 commits into from
Jun 26, 2022
Merged

docs: edits to vignettes #958

merged 6 commits into from
Jun 26, 2022

Conversation

IndrajeetPatil
Copy link
Collaborator

Marking this as a draft for now, since there might be some other edits that I can include in this PR.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 80f58d5 is merged into main:

  •   :ballot_box_with_check:cache_applying: 28.8ms -> 29.4ms [-2.27%, +6.77%]
  •   :ballot_box_with_check:cache_recording: 1.23s -> 1.23s [-0.94%, +0.82%]
  •   :ballot_box_with_check:without_cache: 3.29s -> 3.27s [-1.62%, +0.69%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2022 •
edited
Loading

Codecov Report

Merging #958 (a8b1f61) into main (e0116c8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #958   +/-   ##
=======================================
  Coverage   89.88%   89.88%           
=======================================
  Files          47       47           
  Lines        2609     2609           
=======================================
  Hits         2345     2345           
  Misses        264      264
Impacted Files Coverage Δ
R/ui-caching.R 90.24% <ø> (ø)
R/ui-styling.R 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0116c8...a8b1f61. Read the comment docs.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 094f453 is merged into main:

  •   :ballot_box_with_check:cache_applying: 25.6ms -> 25.5ms [-2.01%, +0.65%]
  •   :ballot_box_with_check:cache_recording: 1.11s -> 1.11s [-1.02%, +0.11%]
  •   :rocket:without_cache: 3s -> 2.98s [-1.02%, -0.11%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil IndrajeetPatil marked this pull request as ready for review June 17, 2022 23:27
@IndrajeetPatil
Copy link
Collaborator Author

@lorenzwalthert This should be good to go.

@lorenzwalthert
Copy link
Collaborator

Thanks for starting this 😎

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if e552415 is merged into main:

  • ❗🐌cache_applying: 22.6ms -> 22.7ms [+0.21%, +1.02%]
  •   :ballot_box_with_check:cache_recording: 967ms -> 963ms [-0.8%, 0%]
  •   :ballot_box_with_check:without_cache: 2.59s -> 2.58s [-0.3%, +0.25%]

Further explanation regarding interpretation and methodology can be found in the documentation.

IndrajeetPatil
IndrajeetPatil commented Jun 24, 2022
View reviewed changes
R/ui-caching.R
@@ -61,7 +61,7 @@ NULL
#'
#' Gives information about the cache. Note that the size consumed by the cache
#' will always be displayed as zero because all the cache does is creating an
#' empty file of size 0 bytes for every cached expression. The innode is
#' empty file of size 0 bytes for every cached expression. The inode is
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://en.wikipedia.org/wiki/Inode

IndrajeetPatil
IndrajeetPatil commented Jun 24, 2022
View reviewed changes
R/ui-styling.R
@@ -1,4 +1,4 @@
#' @api
#' @keywords api
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

roxygen2 doesn't have any such tag, so this leaves a warning.

IndrajeetPatil
IndrajeetPatil commented Jun 24, 2022
View reviewed changes
vignettes/caching.Rmd
@@ -18,11 +18,11 @@ knitr::opts_chunk$set(
library(styler)
```

This is a developer vignette to explain how caching works and what we learned on the way. To use the caching feature, please have a look at the README.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

README doesn't have anything about caching anymore.

@IndrajeetPatil
Copy link
Collaborator Author

@lorenzwalthert I think this is ready for a review.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 11aa626 is merged into main:

  •   :ballot_box_with_check:cache_applying: 22.9ms -> 23ms [-0.36%, +0.72%]
  •   :ballot_box_with_check:cache_recording: 982ms -> 981ms [-0.37%, +0.17%]
  •   :ballot_box_with_check:without_cache: 2.65s -> 2.64s [-0.48%, +0.23%]

Further explanation regarding interpretation and methodology can be found in the documentation.

lorenzwalthert
lorenzwalthert approved these changes Jun 26, 2022
View reviewed changes
Copy link
Collaborator

@lorenzwalthert lorenzwalthert left a comment

Choose a reason for hiding this comment

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

Great. Nothing to add from my side here.

@lorenzwalthert lorenzwalthert merged commit 9b0faa2 into r-lib:main Jun 26, 2022
@lorenzwalthert
Copy link
Collaborator

Thanks 😊

@IndrajeetPatil IndrajeetPatil deleted the edits_to_vignettes branch June 26, 2022 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorenzwalthert lorenzwalthert lorenzwalthert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@IndrajeetPatil @codecov-commenter @lorenzwalthert