data-edu / data-science-in-education

Repository for the second edition of 'Data Science in Education Using R' by Emily A. Bovee, Ryan A. Estrellado, Joshua M. Rosenberg, and Isabella C. Velásquez
http://www.datascienceineducation.com/
260 stars 78 forks source link

Ch 10/write new content #599

Closed restrellado closed 1 day ago

restrellado commented 3 weeks ago

Includes general review plus review for style.

I tested the code and it runs. Still needs a code review for style and a grammar review.

netlify[bot] commented 3 weeks ago

Deploy Preview for datascienceineducation-1ed ready!

Name Link
Latest commit 5d22f56ae57da711d52ff86ab26bee2f70649f8a
Latest deploy log https://app.netlify.com/sites/datascienceineducation-1ed/deploys/6702b00763043f00081da2ea
Deploy Preview https://deploy-preview-599--datascienceineducation-1ed.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jrosen48 commented 1 day ago

Minor -- should we consider using purrr::pluck() for this code?

https://purrr.tidyverse.org/reference/pluck.html

all_files[[5]]

What made me think of this was that we say, "Now that you know the 2016 dataset is the fifth element in the list, pluck it out by using double brackets"!

jrosen48 commented 1 day ago

Another minor code-related thing -- why do we have to quote 2012 here (can it not be treated as an integer)?

model_data %>%
  filter(year == "2012") %>%
  summary()
jrosen48 commented 1 day ago

Very minor - are we using dataset or data set?

jrosen48 commented 1 day ago

Are we avoiding contractions? Aren't, they're, etc.

I think so, but want to confirm.

jrosen48 commented 1 day ago

Also, you'll, you're, here's

restrellado commented 1 day ago

Minor -- should we consider using purrr::pluck() for this code?

https://purrr.tidyverse.org/reference/pluck.html

all_files[[5]]

What made me think of this was that we say, "Now that you know the 2016 dataset is the fifth element in the list, pluck it out by using double brackets"!

Deferring to code guru @ivelasq on this. No strong feelings from me either way. But I won't let the decision block the merge. Happy to open another PR if we decide to do it.

restrellado commented 1 day ago

Very minor - are we using dataset or data set?

For both of these style questions, I posted in Slack to revisit. I'll merge and am happy to open another PR in case we need to make changes for either contractions or "dataset" vs "data set."

restrellado commented 1 day ago

Another minor code-related thing -- why do we have to quote 2012 here (can it not be treated as an integer)?

model_data %>%
  filter(year == "2012") %>%
  summary()

We could probably remove. But I just checked and at that part of the walkthrough year is a factor. That's probably why I used quotes. That may be an outdated way of doing it—let me know if so. I'm going to merge as is, but happy to change if anyone has strong feelings.