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.

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

Don't call init_site() when lazy = TRUE + use posix line end + document init_site() + speed cli console output #2642

Merged
merged 11 commits into from
Jun 7, 2024

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Jun 6, 2024

I am not talking about init_site() anymore after this!

Follow-up to #2571

Only call init_site() if lazy = FALSE

Document where init_site() is actually needed. #2557 (comment)

To speed console printing, create cache_cli_colours(). (bench marks in #2557 (comment)) Tip taken from r-lib/cli#607

I put in the Posix line end thing as a follow-up to r-lib/testthat#1958 (comment)

I am no longer seeing spurious diff in snapshot tests this way.

Still haven't figure out no-pandoc test failure! Will take a look again after #2645. This appears to be stochastic (just encountered it on main)

Copy link

github-actions bot commented Jun 6, 2024

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Looks good!

R/context.R Outdated Show resolved Hide resolved
R/context.R Outdated Show resolved Hide resolved
R/init.R Outdated
@@ -21,6 +29,7 @@
#' @inheritParams build_articles
#' @export
init_site <- function(pkg = ".") {
local_options_console()
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure we shouldn't just callsection_init() here? I also noticed that init_site() lacks override, and I needed it when I started experimenting with quarto.

That would make it more consistent with the other functions

Copy link
Member

Choose a reason for hiding this comment

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

Now clear that it shouldn't be called here 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Hadley, actually, cache_cli_colours() should be used in init_site(),

bench marks

# without caching
 expression    min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result
  <bch:expr>  <bch> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>
1 init_site() 3.23s  3.23s     0.309    14.7MB    0.619     1     2      3.23s <NULL>
# with caching
  expression    min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result
  <bch:expr>  <bch> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list>
1 init_site() 3.02s  3.02s     0.331    8.89MB    0.992     1     3      3.02s <NULL>

Significant difference in memory usage. And also, the output prints much faster in the console!

olivroy added 2 commits June 7, 2024 12:12
Merge commit '8defd983aab0cf0eb9517813ecb4102915c84008'

#Conflicts:
#	R/init.R
pkgdown.Rproj Outdated Show resolved Hide resolved
@olivroy olivroy requested a review from hadley June 7, 2024 17:25
@olivroy olivroy merged commit 2c8d15d into main Jun 7, 2024
@olivroy olivroy deleted the config branch June 7, 2024 19:43
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