-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
new quick example #618
new quick example #618
Conversation
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.
That is really great ! Thanks.
Just few comments
Co-authored-by: Christophe Dervieux <christophe.dervieux@gmail.com>
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.
Thanks.
Going with desc package we may need to add it as a dependency for the book so that it builds on CI. In the action file, or in index.Rmd
with other.
However, I believe it will be available because we install pkgdown so that it is reference.
So two things I need to do later:
- improve the way we manage dependencies for the book
- add a build on PR when book Rmd files are modified so that we know if it fails or not.
Thanks- I'll let @yihui or you merge in, then follow up with additional PRs I have in branches right now to rework some other sections. |
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.
Looks great to me overall! I'm rereading it and making a few small edits now. Thanks!
|
||
``` | ||
```text |
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.
Hi @yihui - the output here no longer really makes sense- I was trying to introduce the checking functions as they applied (so it wouldn't be overwhelming at first)- but this is just the output to check_content()
- you may want to update with the whole output of check_site()
I guess?
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.
Okay, I'll add more lines of output and omit some of them (by using ...
).
Here I avoided mentioning check_content()
specifically because I think it's too much for readers to remember the five specific checking functions in Chapter 1 (especially in a section titled "A quick example"---we want to make it really quick). I hope that makes sense.
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.
That makes sense, we just disagree. I was trying to limit cognitive load b/c there is so much new vocab to learn, and I outlined the sections to map onto the functions to help build a mental model (I had hoped).
But I'm ok with this change- all my later PRs followed the same model, so I'll let you make those adaptations as you go.
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.
I think we agree on the goal but disagree on the means. The goal of introducing check_site()
instead of check_content()
is meant exactly to limit cognitive load and new vocab. It's very unlikely that check_content()
alone would help most users, and we will have to introduce check_site()
at some point in the book, which means users will have to continue digging into the rest of the book, and remember at least two functions, check_content()
and check_site()
. To me, that requires more cognitive load and larger vocab.
To be honest, I was still not convinced that the sub functions of check_site()
need to be exported and used individually. I can't even remember all of them easily myself, and wouldn't use them individually, because when my site is in trouble, it's difficult for me to predict which part went wrong (config? gitignore? content? netlify?), so it'd also be difficult for me to know which specific check_*()
function to run. I think this applies to you, too (e.g. last Friday you didn't realize the problem was from config.yaml
, so you wouldn't be able to know that you had to run check_config()
; even after I told you it was because of ignoreFiles
, you were trying to look at .gitignore
---perhaps you misheard me). The downside of running check_site()
is that it might output some noise (parts that are normal), but the noise can be relatively easily filtered out since the TODO items are prominently marked.
It makes perfect sense to explain what check_site()
does in five parts, but it makes little sense to me to teach users to do these five steps separately. That said, I trust your expertise in teaching more than mine, so I exported these five functions anyway. In practice, I'd use the one and only function to check everything.
Hi @yihui - a new "quick example" to replace: https://bookdown.org/yihui/blogdown/a-quick-example.html