Follow-up for attached packages fix - #1403
Conversation
juliasilge
left a comment
juliasilge
left a comment
There was a problem hiding this comment.
This is working well for me in various workspace configurations!
I have one requested change before you merge, to update one of the user facing messages; I think it will be wrong/confusing to folks as is.
| fn lower_source_cycle(loader: Option<LoaderInfo>) -> Diagnostic { | ||
| let cause = match loader { | ||
| Some(LoaderInfo { name, loads }) => { | ||
| format!("{name} already loads {loads}, so a `source()` call between them is redundant.") |
There was a problem hiding this comment.
This sentence tells the reader to remove a source() call that a Shiny app actually needs to work, right? An alternative would be to just state the fact and stop:
format!("{name} already loads {loads}. A `source()` call into a file that the loader loads later creates this cycle.")Let me know if I am misunderstanding!
There was a problem hiding this comment.
Also related to the message here, loader(db, file) reads the diagnosed file only and IIUC nothing makes sure that the other files in the cycle have the same loader. The message still makes a claim about the relationship between them: "a source() call between them is redundant".
SemanticDiagnostic::SourceCycle is a unit variant, so the participants are not available here. A gate on the shared loader would be pretty hard and needs the participant list in the variant. I think the cheaper, better option is to keep this sentence more descriptive.
There was a problem hiding this comment.
That makes sense! I made it:
This Shiny app already loads its `global.R` and `R/` files through `shiny::loadSupport()`.
A `source()` call into a file that the loader also loads creates this cycle.
It can introduce unexpected cycles that trigger unactionable user lints. Instead we'll strive to model explicit loading mechanisms like we did for targets and shiny.
2070d2d to
895245f
Compare
2 participants
Follow-up for #1393:
R/folder collation load. This addresses R scripts thatsource()a sibling inR/lose their attached packages positron#15665.I decided to go for 3 because of two undesirable consequences when the implicit collation conflicts with an explicit
source()call written by the user:I think that pretty much kills the approach. Potentially we could disable the implicit folder loader for files that have an explicit source, but that behaviour would we hard to understand and predict for users. So I think we're better off modelling explicit source effects, like we did with the Shiny and testthat file loaders, or with
targets::tar_source().Here is an example of the improved lint when an explicit loader conflicts with a user Source effect. Compared to the problematic lint above, this one is both correct and actionable:
Positron Release Notes
Let me know if you'd like a release note item here. It seemed to me mentioning this change would more likely confuse users.
New Features
Bug Fixes