add support for package-provided project templates #908

kevinushey · 2016-11-21T22:40:55Z

NOTE: PR not quite ready for merge, but most components ready for review.

This PR implements support for package-provided project templates. This PR basically provides an interface for RStudio to discover and call various 'skeleton' functions that packages can provide. A brief look:

Packages can define a project template by creating one or more skeleton descriptions in the rstudio/templates folder. The project template descriptions should be DCF files with the form e.g.

Binding: Rcpp.package.skeleton
Title: R Package using Rcpp
Subtitle: Create a new R Package using Rcpp
Caption: Create R package using Rcpp

Parameter: module
Widget: CheckBox

That is, each DCF file should contain an initial DCF record containing information about the skeleton function, and then 0 or more descriptions of widgets that can be used to power the widget.

TODO

Ensure that the index is updated when the library is changed (as we do with addins)
Ensure that errors are handled / reported well. (we might want to consider some kind of diagnostics output when editing / working with a dcf file in the rstudio/templates folder)
Consider generating Subtitle, Caption based on Title field.
Allow for package skeletons provided in ~/.R/rstudio/templates directory?
Add support for icons.
Ensure that the project template registry is received on browser refresh / reload.
Add support for default values / selections for select inputs?
...

kevinushey · 2016-11-21T22:41:52Z

src/cpp/core/include/core/text/CsvParser.hpp

@@ -138,6 +138,19 @@ std::pair<std::vector<std::string>, InputIterator> parseCsvLine(
         std::vector<std::string>(), begin);
 }

+template <typename InputIterator>
+InputIterator parseCsvLine(InputIterator begin,


I added this overload primarily because the main version that returns a std::pair<> can often be cumbersome to work with.

jmcphers · 2016-11-22T00:16:54Z

src/cpp/session/include/session/SessionProjectTemplate.hpp

+         LOG_ERROR(error);
+
+      core::json::fillVectorString(
+            object["fields"].get_array(),


IIRC this will throw if the object does not contain fields, and also if fields is not an array, so you might consider a two-stage access here for safety (get the JSON array above then coerce to a string vector here)

jmcphers · 2016-11-22T00:18:11Z

src/cpp/session/include/session/SessionProjectTemplate.hpp

+               "label",     &ptwd.label);
+
+      if (error)
+         LOG_ERROR(error);


I think it'd be better here to return the error (and take a pointer to a ptwd as a parameter) so that the caller knows not to process the template if it's malformed.

Good idea -- I'll make that change.

jmcphers · 2016-11-22T00:22:50Z

src/cpp/session/include/session/SessionProjectTemplate.hpp

+         else if (key == "Icon")
+         {
+            // read icon file from disk
+            core::FilePath iconPath = resourcePath.parent().complete(value);


It might be worth defending against pathologically large files here by skipping icons larger than some reasonable size (we don't want to accidentally try to stream megabytes of base64 data to the client).

jmcphers · 2016-11-22T00:24:28Z

src/cpp/session/include/session/SessionProjectTemplate.hpp

+      if (error)
+         LOG_ERROR(error);
+
+      core::json::Array widgetsJson = object["widgets"].get_array();


As above w/ fields (this can throw exceptions)

jmcphers · 2016-11-22T00:30:17Z

src/cpp/session/modules/SessionProjectTemplate.cpp

+private:
+   void onIndexingStarted()
+   {
+      pRegistry_ = boost::make_shared<ProjectTemplateRegistry>();


It looks like this shared pointer manages the lifetime of the globalProjectTemplateRegistry. If that's the case then does resetting it cause the pointer to release the registry, so that there is briefly no registry until indexing completes?

Not quite -- the indexer actually contains its own registry (and maintains a shared pointer to that); after indexing has completed the global registry is updated based on that reference.

Does the object ever actually get copied, though? If not the global registry is a reference to an object on the heap that's owned only by the shared pointer. Let's discuss realtime.

After discussion -- indeed you're right! The global registry needs to be represented as a shared pointer; in addition I'm missing a noncopyable inheritance on the registry class. Thanks for pointing this out!

jmcphers · 2016-11-22T00:32:19Z

src/cpp/session/modules/SessionProjectTemplate.cpp

+         return Success();
+
+      std::string reason =
+            "invalid project template description: missing or empty fields "


Could we also log the location of the file in which these fields were expected? You can just add this as a property on the error; otherwise I think the message in the logs will be hard to associate with a particular package.

jmcphers · 2016-11-22T00:32:56Z

src/cpp/session/modules/SessionProjectTemplate.cpp

+         pDescription->subtitle = "Create a new " + title;
+
+      if (pDescription->subtitle.empty())
+         pDescription->caption = "Create " + title;


Nit: dead code?

Whoops! This should be checking the caption field; thanks for catching this.

jmcphers · 2016-11-22T00:47:23Z

I think many project skeletons are going to wind up including sample files with boilerplate code. For some projects it will be necessary to run R code to generate these, but I think most can just include them declaratively. We might want to add facilities for:

Indicating a path to a directory tree of files which will be copied into the new project by convention (bonus points if we allow some simple templating or string substitution in file names and/or contents)
Indicating a set of files that should be open when the new project first starts up; this is a common user request and we already have machinery for it (it's done for e.g. new Shiny projects)

Also worth considering:

What mechanism would you recommend to an RStudio Server admin who wants to supply their users with templates? (Of course they could make an R package and install it somewhere in the global library, just wondering if there's something else we could do with less overhead)
Could we implement some of our existing new project templates using this system? I think that at a minimum it would be pretty straightforward to do this with Shiny, especially if we have some package-less way of discovering templates (would also help w/ above)

kevinushey · 2016-11-22T03:50:09Z

Indicating a path to a directory tree of files which will be copied into the new project by convention (bonus points if we allow some simple templating or string substitution in file names and/or contents)

I'm slightly more inclined to delegate this responsibility to the skeleton function; any reason in particular why we should take control here?

Indicating a set of files that should be open when the new project first starts up; this is a common user request and we already have machinery for it (it's done for e.g. new Shiny projects)

Definitely a good idea!

What mechanism would you recommend to an RStudio Server admin who wants to supply their users with templates? (Of course they could make an R package and install it somewhere in the global library, just wondering if there's something else we could do with less overhead)

I was also thinking of parsing project templates in a preset folder, e.g. ~/.R/rstudio/templates, but I'm open to suggestions here as well. We might also allow this path to be configured in other ways -- a UI preference, an environment variable, but I'm not yet sure what would be best.

Could we implement some of our existing new project templates using this system? I think that at a minimum it would be pretty straightforward to do this with Shiny, especially if we have some package-less way of discovering templates (would also help w/ above)

Yes, I think the Shiny package skeleton we have could be replaced with a pure project template version. However, note that currently all of these skeleton functions create a brand new project; the 'New Shiny Application' dialog merely creates some new files (ie it's less heavyweight).

jjallaire · 2016-11-22T13:21:05Z

Some top-level comments prior to code review:

I agree that the skeleton function should be the one that does the file copying, substitution, etc. This enables these functions to work both within and outside of the IDE. This however does make it slightly harder for less sophisticated developers (e.g. people creating project templates inside a company) to implement a package template. Maybe we can just provide some example code as part of the documentation to ease this along.
Major +1 to specifying files to open! Note that in some cases the names of files will be dynamic based on variables (including the name of the project, I'm thinking here of what happens in htmlwidgets::createWidget) so we may need some simple variable substitution within the DCF file.
In terms of centralized deployment, I think that we should continue to rely on packages for this rather than defining a preset folder. This is because these templates are intended to execute code and to me code should be deployed in a package. Note that for addins and R Markdown templates (not even to speak of just common/shared libraries!) you already need to centrally deploy packages so I think this is something that every moderately sophisticated environment will have a handle on (and we can certainly help this along with documentation and product features).
Agree w/ Kevin that the "New Shiny App" doesn't quite fit b/c it doesn't create a package. Note that Kevin and I have discussed extending this framework to "New File" however that's on the bubble for this release (have other things that might be more pressing). I do think that we should implement some templates "internally" (e.g. Rcpp, devtools, htmlwidgets, etc.) and then have a mechanism where we can start delegating to the package once it provides a DCF file.
I think the directory should be rstudio/templates/project to accommodate the possibility of non-project (e.g. file) templates.

kevinushey · 2016-11-22T18:31:28Z

Whoops, yes -- sorry, I flipped the order!

kevinushey · 2016-11-22T19:09:08Z

Some thoughts on how the user might specify what files should be opened when the new project is opened:

OpenFiles: R/server.R, R/ui.R, README-%s

Ie, the OpenFiles field can be a CSV line; the paths will be resolved relative to the new project root. The simplest form of templating we could do is just substitute e.g. %s with the project name (name of created folder); any thoughts on whether we want to be more opinionated / provide something a bit different? Some other candidates:

1) OpenFiles: README-${PACKAGE}     # shell-style
2) OpenFiles: README-{{ package }}  # whisker-style
3) OpenFiles: README-%s             # sprintf-style

One thing that would be nice about an environment-variable based substitution -- the skeleton function could just set some environment variables that would could then be read by RStudio when forming the template file paths.

We could also consider supporting globs, e.g.

OpenFiles: R/*.R

jjallaire · 2016-11-22T19:15:57Z

I'd say support globs and support shell-style variables. Variables should
include the package name and any of the other input variables specified.

On Tue, Nov 22, 2016 at 2:09 PM, Kevin Ushey notifications@github.com
wrote:

Some thoughts on how the user might specify what files should be opened
when the new project is opened:

OpenFiles: R/server.R, R/ui.R, README-%s

Ie, the OpenFiles field can be a CSV line; the paths will be resolved
relative to the new project root. The simplest form of templating we could
do is just substitute e.g. %s with the project name (name of created
folder); any thoughts on whether we want to be more opinionated / provide
something a bit different? Some other candidates:

OpenFiles: README-${PACKAGE} # shell-style

OpenFiles: README-{{ package }} # whisker-style

OpenFiles: README-%s # sprintf-style

One thing that would be nice about an environment-variable based
substitution -- the skeleton function could just set some environment
variables that would could then be read by RStudio when forming the
template file paths.

We could also consider supporting globs, e.g.

OpenFiles: R/*.R

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#908 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGXx9rLLMGLFiKInzrYYJZ6dnYzSTWKks5rAz3UgaJpZM4K4ydg
.

…plate-indexer

kevinushey · 2016-11-29T22:23:58Z

src/cpp/core/include/core/json/JsonRpc.hpp

+}
+
+template <typename T>
+core::Error readObject(


This method (+ the one above) allow us to more easily read vectors of things, e.g. vectors of strings.

…plate-indexer

kevinushey · 2016-12-20T22:03:10Z

I think this PR is ready for a more formal code review now. @jmcphers, if you have time would you be willing to take a look?

jmcphers · 2016-12-20T23:48:11Z

src/cpp/session/projects/SessionProjects.cpp

-   else
+   
+   // if we have a custom project template, call that first
+   if (!projectTemplateOptions.is_null())


for sanity, consider making sure its type is json::Object here too (otherwise it looks like initializeProjectFromTemplate can throw)

jmcphers · 2016-12-20T23:54:54Z

src/gwt/src/org/rstudio/studio/client/projects/model/ProjectTemplateRegistryProvider.java

+
+      // execute pending callbacks
+      for (Callback callback : pendingCallbacks_)
+         callback.execute(registry_);


Nit: Could double evaluate if a callback throws while executing -- maybe wrap in a try catch and/or remove the callback before executing?

kevinushey · 2016-12-21T18:03:14Z

@jjallaire, do you want to review / sanity check before merge? (We can also tweak + update things on master as needed)

jjallaire · 2016-12-21T19:36:02Z

I'm good, will merge now.

…

On Wed, Dec 21, 2016 at 1:03 PM, Kevin Ushey ***@***.***> wrote: @jjallaire <https://github.com/jjallaire>, do you want to review / sanity check before merge? (We can also tweak + update things on master as needed) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#908 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGXx1j6vI7NfAb4n14TStO8F2FYul0Qks5rKWnigaJpZM4K4ydg> .

kevinushey added 12 commits November 16, 2016 10:30

initial work on indexer for project template files

96e8d52

initial work on wizard page (mostly stubs)

72d7c67

eclipse warnings

a819988

provide preferred entrypoint to retrieving project templates

9fe656a

initial work on populating navigation list of wizard pages

07ea29c

begin work on template-aware navigation page

dee6dda

clean up; infrastructure for custom icons

ee3f939

add ability to pass along project template options

8bcf619

wire up project creation with template

c740e39

initial work on widget description

ba98b41

more work on parsing of widget records

4070e18

end-to-end wiring of skeleton inputs

efa4e18

kevinushey commented Nov 21, 2016

View reviewed changes

kevinushey added 3 commits November 21, 2016 15:10

emit client event when indexing completed, UI tweaks

00c2797

use json::readObject when appropriate; other cleanup

691c260

provide icons as base64-encoded data

a67f72e

jmcphers reviewed Nov 22, 2016

View reviewed changes

kevinushey added 3 commits November 21, 2016 19:51

tidy up (more error information; copy-paste error)

3f171af

move some implementation code out of header

55113d5

further tidy up

81cb5d0

kevinushey added 5 commits November 22, 2016 10:18

further clean up JSON (de)serialization

d10b540

maintain global registry as shared ptr

e960af2

remove unused validation routine (need to move elsewhere)

cc77c74

index in 'rstudio/projects/templates' folder

c780057

index in 'rstudio/templates/project'

b59715a

kevinushey added 16 commits November 22, 2016 14:48

add support for opening files on startup

69727d5

client now requests project template registry on load

be93ac7

tidy + case-insensitive regex

317b8c8

move skeleton invocation to R side

3dbfe61

move widget types to implementation file; use vector

cfae2d6

Merge remote-tracking branch 'origin/master' into feature/project-tem…

fbe2719

…plate-indexer

fix up type mismatch error report

11813ef

support non-package project templates

eea6c9c

tweak widget names (match Shiny)

2ec09cb

improvements to error reporting

ff9dba7

some client-side tidying up

6b61fb6

add support for default fields

8de2b44

add support for 'Position' field (left vs. right column)

6151cec

add basic error reporting for project templates

4b09b85

log user errors only to console

7b8a109

handle casing for 'position' parameter

2950bb9

kevinushey commented Nov 29, 2016

View reviewed changes

kevinushey added 3 commits December 20, 2016 13:41

Merge remote-tracking branch 'origin/master' into feature/project-tem…

d42287a

…plate-indexer

fix indentation (vertical alignment)

384127e

null check in collector use

fc53bad

jmcphers approved these changes Dec 21, 2016

View reviewed changes

kevinushey added 2 commits December 20, 2016 17:29

ensure object type for project template options

1494f94

catch potential exceptions when executing callbacks

6ee72b3

jjallaire merged commit 2ca163a into master Dec 21, 2016

valerie-rstudio deleted the feature/project-template-indexer branch January 21, 2022 17:22

gtritchie mentioned this pull request Mar 16, 2022

Update SOCI dependency to 4.0.3 #10792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for package-provided project templates #908

add support for package-provided project templates #908

kevinushey commented Nov 21, 2016 •

edited

kevinushey Nov 21, 2016

jmcphers Nov 22, 2016

jmcphers Nov 22, 2016

kevinushey Nov 22, 2016

jmcphers Nov 22, 2016

jmcphers Nov 22, 2016

jmcphers Nov 22, 2016

kevinushey Nov 22, 2016

jmcphers Nov 22, 2016

kevinushey Nov 22, 2016

jmcphers Nov 22, 2016

kevinushey Nov 22, 2016

jmcphers Nov 22, 2016

kevinushey Nov 22, 2016

jmcphers commented Nov 22, 2016 •

edited

kevinushey commented Nov 22, 2016 •

edited

jjallaire commented Nov 22, 2016

kevinushey commented Nov 22, 2016

kevinushey commented Nov 22, 2016

jjallaire commented Nov 22, 2016

kevinushey Nov 29, 2016

kevinushey commented Dec 20, 2016

jmcphers Dec 20, 2016

jmcphers Dec 20, 2016

kevinushey commented Dec 21, 2016

jjallaire commented Dec 21, 2016 via email

add support for package-provided project templates #908

add support for package-provided project templates #908

Conversation

kevinushey commented Nov 21, 2016 • edited

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcphers commented Nov 22, 2016 • edited

kevinushey commented Nov 22, 2016 • edited

jjallaire commented Nov 22, 2016

kevinushey commented Nov 22, 2016

kevinushey commented Nov 22, 2016

jjallaire commented Nov 22, 2016

Choose a reason for hiding this comment

kevinushey commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinushey commented Dec 21, 2016

jjallaire commented Dec 21, 2016 via email

kevinushey commented Nov 21, 2016 •

edited

jmcphers commented Nov 22, 2016 •

edited

kevinushey commented Nov 22, 2016 •

edited