-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix passing DBI::Id
to update_snapshot
and dbplyr messages
#72
Conversation
60029b7
to
c6b9b9a
Compare
Revert changes from 5406fa3
77a25b8
to
7f474f1
Compare
6122409
to
78638e2
Compare
chore: Remove SQLite-specific functions from `get_tables.default`
78638e2
to
e898c27
Compare
cf41b23
to
cc417ee
Compare
12c796e
to
d8f8f02
Compare
This comment was marked as outdated.
This comment was marked as outdated.
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.
Firstly, there a few places where it looks like you have made minor mistakes.
Secondly, I have some general comments, tips etc.
Finally, I have some pedantic comments you can take or leave as you please.
This change currently has no race condition prevention, and multiple threads calling `log_to_db` simultaneously will therefore crash.
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 fine to me.
Just need the last check to work, but that should not be an issue.
Intent
This fixes #68.
Additionally, while fixing, I managed to squash all the messages about
in_schema
caused by dbplyr v2.4.0 giving this warning VERY often1.Approach
update_snapshot
had some "leaky" logic, not properly catching all cases. Now, the addition ofid.tbl_dbi
allows for easy retrieval of a table ID to use onwards. As we now do not expect anydb_table
arguments to not be passable byid
, this will catch all valid inputs.Regarding the numerous messages from dbplyr, I hope that they will be changed to a more graceful form, but for now, all calls to
dplyr::tbl
anddplyr::copy_to
in tests and code have been changed to includecheck_from = FALSE
.Also, some instances of above messages were caused by the
Logger
unnecessarily copying a data.frame twice in one log update.Known issues
I concede that passing
check_from = FALSE
to all calls mentioned above is a somewhat lazy solution. One solution that I thought of just now (and that I could honestly see working better) is to implement some kind of check to see if the caller env is.GlobalEnv
and give a warning at most ONCE per session.The current state of
logger$log_to_db
has no race condition prevention. This is neither introduced nor fixed by this PR, but race condition prevention is an overall topic of #63 and therefore outside the scope of this PR.Checklist
NEWS.md
2Footnotes
Naturally, you will not find me missing an opportunity to brag that I managed to identify—and fix—the issue myself. ↩
In order to not mangle
NEWS.md
too much, this will be done after v0.2.1 is accepted on CRAN. ↩