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 mention iris dataset in documentation #752

Merged
merged 7 commits into from
Aug 16, 2024

Conversation

fontikar
Copy link
Contributor

Addresses #642

Problem: iris dataset is stored in dataset.xlsx and dataset.xls. iris is used in various examples to illustrate the functionality of readxl, the goal is to remove this iris or replace it with another nicer dataset e.g. penguins 🐧

After chats with Garrick and Emil, they suggested the smallest scoped solution is to remove any public facing documentation is uses iris and ignore the test files that uses iris.

@drorberel and I decided to remove the iris spreadsheet and update the README files, vignettes and any examples in .R that refer to iris.

Hope that we didn't cause any downstream breakages! 😄

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

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

Looking good! There's one example I hope we can tweak.

R/read_excel.R Outdated
#'
#' # Recycle a single column type
#' read_excel(datasets, col_types = "text")
#'
#' # Specify some col_types and guess others
#' read_excel(datasets, col_types = c("text", "guess", "numeric", "guess", "guess"))
#' read_excel(datasets, col_types = rep("guess", 11))
Copy link
Member

Choose a reason for hiding this comment

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

This change directly contradicts the comment just above it. Can you target a different worksheet and hold on to the intent of the example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So true! Thanks for pointing this out 😊 I've updated this now with a better example!

@jennybc
Copy link
Member

jennybc commented Aug 16, 2024

Thanks!

@jennybc jennybc merged commit 35a1905 into tidyverse:main Aug 16, 2024
11 checks passed
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