-
Notifications
You must be signed in to change notification settings - Fork 83
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
Respect renv lockfiles #791
Changes from 56 commits
462da70
8a928ea
3796044
0928e7a
5dbb07c
24fe5ab
024be49
8c7a828
93e6522
46d3c05
9fe6c68
88aeced
adb2b84
5dfcb80
e4fcac7
3a60c58
5b8f9b5
ef4e237
fedb322
b93ae24
3c0f2a6
52d5fd9
1386887
8129386
c29339b
f7d42fd
de2efd6
d83f36e
156e637
f8a7d71
f868fe3
91c8a24
0f77dd2
4bee9a5
f014634
b667f1f
3cbe9e0
ae60d88
e81c51e
48c4e6a
08fff5d
c6a88cd
bfaa7ea
b386d8a
1c6cef8
2d4c4d3
8fe29c6
957c1a5
5198d45
36d0d50
af72cd0
d7db6dd
f53703d
3ec221a
48ecfa5
6e7a8b2
752857f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -179,7 +179,7 @@ ignoreBundleFiles <- function(dir, contents) { | |
# rsconnect packages | ||
"rsconnect", "rsconnect-python", "manifest.json", | ||
# packrat + renv, | ||
"renv", "renv.lock", "packrat", | ||
hadley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"renv", "packrat", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but I don't think that should matter because the settings only really affect interactive usage, and the consequence of those settings will be encoded in the lockfile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The recommendation to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this cause There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes; that's necessary so that we can process it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am torn on this one. On the one hand, including only the On the other hand, I like including the Mostly, my struggle is that I had thought about this change as "manifest-only, no packrat.lock" rather than "manifest plus renv.lock; except when we create the renv.lock on the fly". Let's go forward with the (sometimes) inclusion, but probably document the change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like it would be a bit more consistent to include lock file, even when we generate it? I'm currently explicitly deleting it, but it seems reasonable to remove that code and preserve the lock file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A generated lock file isn't on the set of "bundle files" that was computed much earlier in the workflow... I'm OK either way, but if we do include a generated lock file, let's include it only when we have tracked dependencies (e.g. not for static). |
||
# version control | ||
".git", ".gitignore", ".svn", | ||
# R/RStudio | ||
|
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.
Do you think that folks will need an explicit snapshot type for deploys and another snapshot type for all other workflows? Put another way: Do we need a way to toggle the snapshot type just for deploys?
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.
renv::snapshot()
does accept atype
argument so this would be straightforward to wire up, at least.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.
My sense is that once we enable support for renv lockfile, we'll get quite a few feature requests so we might as well just let them accumulate for a while to get a sense for what people actually want, rather than doing too much prospectively.