-
Notifications
You must be signed in to change notification settings - Fork 48
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
Docs: edits to copy and modify vignettes #1098
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.
Thanks, looks good!
@@ -138,7 +141,7 @@ dm_insert_out <- | |||
dm_rows_insert(dm_insert_in) | |||
`````` | |||
|
|||
This gives us a warning that changes will not be persisted. | |||
This gives us a warning that changes will not persist (i.e., they are temporary). |
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.
Is this still true?
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 like it:
library(DBI)
library(dm)
library(tidyverse)
parent <- tibble(value = c("A", "B", "C"), pk = 1:3)
child <- tibble(value = c("a", "b", "c"), pk = 1:3, fk = c(1, 1, NA))
demo_dm <-
dm(parent = parent, child = child) %>%
dm_add_pk(parent, pk) %>%
dm_add_pk(child, pk) %>%
dm_add_fk(child, fk, parent)
sqlite_db <- dbConnect(RSQLite::SQLite())
demo_sql <- copy_dm_to(sqlite_db, demo_dm, temporary = FALSE)
new_parent <- tibble(value = "D", pk = 4)
new_child <- tibble(value = "d", pk = 4, fk = 4)
dm_insert_in <-
dm(parent = new_parent, child = new_child) %>%
copy_dm_to(sqlite_db, ., temporary = TRUE)
dm_insert_out <-
demo_sql %>%
dm_rows_insert(dm_insert_in)
#> Not persisting, use `in_place = FALSE` to turn off this message.
dbDisconnect(sqlite_db)
Created on 2022-06-19 by the reprex package (v2.0.1.9000)
Co-authored-by: Kirill Müller <[email protected]>
Co-authored-by: Kirill Müller <[email protected]>
@krlmlr This should be ready for merge. Let me know if you see any new issues. |
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.
A few comments. Thank you!
Co-authored-by: Kirill Müller <[email protected]>
I think there are no outstanding review comments for this PR now. |
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, should we do another round?
vignettes/howto-dm-rows.Rmd
Outdated
@@ -18,25 +18,25 @@ source("setup/setup.R") | |||
|
|||
|
|||
This tutorial introduces the methods {dm} provides for modifying the data in the tables of a relational model. | |||
There are 6 methods: | |||
There are 5 methods: | |||
|
|||
* [`dm_rows_insert()`](#insert) - adds new rows |
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.
dm_rows_append()
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.
Should all instances of dm_rows_insert()
be replaced by dm_rows_append()
?
Or should the vignette mention both of these options?
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.
We have both, but dm_rows_insert()
seems to be much less useful than dm_rows_append()
.
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, in that case, I think we should only mention dm_rows_append()
?
The fewer functions the user needs to learn about the better, I think, especially if two functions are doing the same thing.
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.
They are different, though -- dm_rows_insert()
will ditch duplicates or give an error, in some cases this is what we want. We can take this on in a separate PR though.
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, missed that. I'll review with dm_rows_append()
added.
vignettes/howto-dm-rows.Rmd
Outdated
@@ -18,25 +18,25 @@ source("setup/setup.R") | |||
|
|||
|
|||
This tutorial introduces the methods {dm} provides for modifying the data in the tables of a relational model. | |||
There are 6 methods: | |||
There are 5 methods: | |||
|
|||
* [`dm_rows_insert()`](#insert) - adds new rows |
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.
We have both, but dm_rows_insert()
seems to be much less useful than dm_rows_append()
.
When called on a whole dm object (without zoom), `compute()` materializes all tables into new (temporary or persistent) tables by executing the associated SQL query and storing the full results. | ||
Depending on the size of your data this may take considerable time or be infeasible. | ||
When called on a whole `dm` object (without zoom), `compute()` materializes all tables into new (temporary or persistent) tables by executing the associated SQL query and storing the full results. | ||
Depending on the size of your data, this may take considerable time or may even be unfeasible. |
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.
unfeasible is gaining traction, but infeasible doesn't seem to be wrong: https://books.google.com/ngrams/graph?content=infeasible%2Cunfeasible&year_start=1800&year_end=2019&corpus=26&smoothing=3&direct_url=t1%3B%2Cinfeasible%3B%2Cc0%3B.t1%3B%2Cunfeasible%3B%2Cc0
Is this a UK/US thing?
Thanks! |
No description provided.