Skip to content
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

Add RStudio addin for interactively creating a new post from a template #12

Merged
merged 40 commits into from
Jun 29, 2021
Merged

Add RStudio addin for interactively creating a new post from a template #12

merged 40 commits into from
Jun 29, 2021

Conversation

mccarthy-m-g
Copy link
Contributor

@mccarthy-m-g mccarthy-m-g commented Jun 5, 2021

Cool package! I'm in the midst of creating a distill blog and really like the vision of this. I wanted to contribute with something users will likely find helpful:

I've created an RStudio addin that allows users to create a new post from a template interactively (similar to the new post addin for {blogdown}), which makes up the bulk of the changes in this pull request. It's essentially just a wrapper for create_post_from_template().

There are also a few other additions:

  • A new exported function, available_templates(), which lists a new default template located in distilltools/inst/rmarkdown/templates/default/skeleton/skeleton.Rmd, as well as any templates located in the following path in a user's website project inst/rmarkdown/templates/template_name/skeleton/skeleton.Rmd (edit: or a user defined path). This was mainly made as a helper for the addin, and it makes some assumptions about how a user structures the path to their templates. I chose this path structure since it is in line with how you add R Markdown templates to R packages.

  • Testing using {testthat} for available_templates() and create_post_from_template(). Although the addin itself cannot be tested with testthat, these provide the necessary coverage. The interactive part has to be checked manually (I did this with my own distill site and it seems to work without error, but you might want to check too in case I missed anything).

  • I added myself as a contributor to the DESCRIPTION.

@mccarthy-m-g mccarthy-m-g mentioned this pull request Jun 5, 2021
@EllaKaye
Copy link
Owner

EllaKaye commented Jun 5, 2021

Hi Michael,

Thank you so much for your interest and enthusiasm for distilltools, and this big contribution to it 😄

I have a really busy week ahead so it might be a few days before I have a chance to review this PR properly, but I wanted to reply ASAP to say that I'd seen it, that I really appreciate it, and that I'll get back to you properly as soon as I can.

@mccarthy-m-g
Copy link
Contributor Author

Hi Ella,

You're welcome. I hope these are helpful 😄

Thanks for letting me know. I also added a new commit that allows users to set the template directory path available_templates() looks in, so it's a bit more flexible now.

@EllaKaye
Copy link
Owner

EllaKaye commented Jun 6, 2021

Hi Michael,

Yes, this is definitely helpful, and, as you've since noticed, something that other users have requested too 😄 I've had a first look at some of this and overall really like what you've done. I have a few comments:

  1. This is a substantial contribution and warrants you listing yourself as full author on the package, rather than just contributor (in general I am following the guidelines in this Stack Overflow thread and the related paper on recognising contributions).
  2. Thanks for adding tests! It's something I want/need to learn about, and have been meaning to add to {distilltools}, and I appreciate you spotting this absence and stepping up to fill it!
  3. The addin seems to work pretty well, except that when I load it up, both on my personal website and on a test website, I get both default and NA listed in the drop-down menu as possible templates, and when I run the addin, even when I select the default I get a warning in the console "In distilltools::available_templates() :
    No skeleton.Rmd file exists for the following templates: NA. See ?available_templates for the expected path structure." When I run available_templates, it gives "/skeleton/skeleton.Rmd" as the path to the NA template. If I fire up the addin from within another project, such as distilltools itself, then there's just the default template, though of course the addin doesn't work because create_post_from_template needs to be called from within a distill site. Also, as soon as I added another template with your specified file structure, the NA one went away too.
  4. It's really nice that the template drop-down provides a list of available templates (rather than requiring the user to type the path). I totally get why you did what you did r.e. location of the templates mirroring how templates in packages work, and it's a totally valid approach, but I think it might be overly complicated. Packages need to do that to make the templates available to package users, but within a personal distill site, there's no reason to go so deeply nested. One of the things I really like about distill, as opposed to Hugo, is the simplicity of the file structure. For my site, I simply have a folder templates in my root directory. I don't find the inst/.../skeleton.Rmd approach super-intuitive, and this is as someone with familiarity in using it when writing my own packages. I wonder if it might be a bit overwhelming for less experienced R users? Another downside to the inst/... approach is that it makes it harder to use the create_post_from_template function in the console, as the path to type in is so much longer. I do appreciate that you have provided an alternative with the Rprofile approach, and that you documented it really well 😄 but I couldn't get that working. I tried running options("distilltools.templates.path" = "custom/template/path/") in the console (with my actual path!) as well as creating a project .Rprofile, tried restarting R etc, but I couldn't get the addin to see my templates in my templates folder.
  5. Given the above, it would be my preference for available_templates and the addin to look for templates in a templates folder in the root directory, documenting well that that's the expected location, but providing an option for a (more nested) alternative if the user prefers.
  6. That said, this issue is complicated by potentially incorporating create_post_from_template into the {distill} package itself, possibly wrapping its functionality into distill::create_post by adding an additional path argument. At the moment, distill::create_post provides a template and, because of that, I don't think distilltools::create_post_from_template needs to (if the user didn't have their own template in mind, there's no reason for them to use that function over the distill one!). To future-proof the addin, it might be worth thinking about it as if it were a wrapper for distill::create_post with an additional path argument. Then, you'd want the default to be the distill default, but also find the user's templates, ideally (as in point 6) in a templates folder. Of course, the authors of distill may have further opinions on this if implemented in their own package, which could complicate things further.

Anyway, that's just some thoughts on what I've had a chance to look at so far. Overall, I think it's really good and you've obviously done lots of work on this and I definitely think we can work out the above to get this PR incorporated into the package. I look forward to hearing your thoughts.

@mccarthy-m-g
Copy link
Contributor Author

Thanks Ella! The feedback was very helpful and I agree with most of it. I've pushed some new commits that address the feedback:

  1. Done. :)
  2. You're welcome! If you haven't read it yet, "R Packages" has a good chapter on testing.
  3. This was an oversight in how the function searched for templates. I've made the searching more robust and flexible now, and also made it so the function only returns the default template in cases where the user hasn't supplied any (so no more NA).
  4. I agree, simpler defaults make more sense 😄 I've made ./templates the default now. But the changes I made allow more complex and arbitrary structures through options() too if the user wants that. I've also tried to make the documentation for all this in ?available_templates clearer, and added an example showing how you can access the path to a template using its name. Could you please check if the available_templates() recognizes your templates now? It works locally for me, as well as in the testthat tests, so it might have been an issue of the documentation being unclear (or my code being fragile) when you tried before.
  5. See 4.
  6. I think supplying a default in available_templates() is useful, as it means the addin (which populates the drop down using the output from available_templates()) can still be used even if the user does not want to supply their own template. There are likely users who would just prefer a GUI option over typing in the console to create a new post, and distill does not provide this functionality currently. Where the default comes from is up for debate. I chose to put one in distilltools because the default created by distill::create_post() does not have the self-contained: false line in the YAML, and the distill docs gave me the impression this was necessary for blog posts (but maybe I'm wrong here). Otherwise it's the same template that's made by distill:create_post(). That said, it's super easy to change the default to the one from distill if that makes more sense!
  7. (continuing from some of your points in 6) Since distilltools::create_post_from_template() is essentially just distill::create_post() with an additional argument for the template path, the addin code is already future-proofed in the case that distilltools::create_post_from_template() is deprecated in favour of a distill::create_post with an additional path argument. We would just need to change which function is referred to in the addin 😄 So unless the distill authors wildly change distill::create_post I don't think there's anything to worry about with the addin code .

@EllaKaye
Copy link
Owner

EllaKaye commented Jun 7, 2021

Hi Michael,

Thanks for making those changes 😄 I'm glad we're on the same page about things!

Again, it may be a few days before I have a chance to review in full, but I'll be sure to get back to you as soon as I do.

In the meantime, one small additional commit, please - can you bump the version number to 0.0.0.9002 in DESCRIPTION? Thanks!

@mccarthy-m-g
Copy link
Contributor Author

Done. Talk soon! 😄

@EllaKaye
Copy link
Owner

Hi Michael,

Thanks for these changes 😄 and all your work on this. I think this is getting really close and I'm excited about launching this feature soon!

I think that the default path for templates could be even simpler. Can we just have a folder in the root directory called templates and then have files called, say, talk-template.Rmd, post-template.Rmd (or whatever the user wants) inside it, doing away with one layer of nesting? Sorry if I wasn't clearer about that being what I had in mind in my last set of feedback. If possible, I like to avoid having files with the same name in the same project, and as it currently is, users may end up with several skeleton.Rmd files. (it used to drive me nuts on my Hugo site having so many index.Rmd files and keeping track of all my tabs in RStudio!). Also, I like my file names to make it clear what the file actually is, and skeleton.Rmd is a little unintuitive for me. One of my overarching concerns for distilltools is making things as simple/straight-forward/intuitive for the users.

Also, I double-checked the build, and saw 0 errors, 0 warnings and 0 notes, so very big thumbs up on that 👍

Hopefully we can get this wrapped up in the next week or so. I should next have a chance to review changes at some point over the weekend.

@mccarthy-m-g
Copy link
Contributor Author

Hi Ella,

You’re welcome! Looking forward to launching it! 😄

I probably won’t have time to make any changes until closer to the end of the week. The current implementation actually lets you name the .Rmd files anything you’d like, so users can avoid generic names if they dislike them, or use generic names if they do like them. I noted this in the function documentation. Would making that more clear in the documentation satisfy your needs as a user? (rather than changing how the function works).

If we were to change the current function to look directly in templates for the R Markdown files, rather than in subdirectories within templates, then the function’s inner workings will probably need to become more complicated if we still want to support users who want generic names for their .Rmd files (and I think it’s important we do). Subdirectories within templates will be supported no matter what (it’s already supported, and I think it’s an important feature to keep in), but the way the path is named by available_templates() will need to change.

Right now the function uses the subdirectory name to name the path. This keeps the code fairly simple, since every subdirectory in templates has to have a unique name. If we move over to using the .Rmd file’s filenames this complicates things for users who want to put their templates in different subdirectories, and to have those templates share the same generic name (e.g., templates/cool-template/template.Rmd and templates/my-template/template.Rmd). The issue here is they will all end up having the same name, so, e.g., available_templates()[[“Template”]] will return paths for both of those, which will likely throw an error if the user selected “Skeleton” in the new post from template addin (and if not will still result in unexpected behaviour).

The workaround for this I can think of is to check if multiple templates in subdirectories share the same name, and if so then add the subdirectory name onto the shared name in some way (or replace the shared name with the shallowest subdirectory name). But I feel that this will overcomplicate the code, which also means overcomplicating the documentation for available_templates() to reflect this so users know what to expect.

So I would prefer to keep the function as is if you have no objections (unless you have alternative suggestions on how to implement your suggested change without restricting users or overcomplicating things in the ways mentioned above). I think having a, e.g., ./templates/my-template/my-template.Rmd path is simple enough (this is what we have right now), and making it simpler will ironically end up making it more complicated as far as I can tell.

Let me know what you think and we can figure out where to go from here!

@EllaKaye
Copy link
Owner

Hi Michael,

Thank you very much for your comments and thoughts above. It's really helpful to have some more of your explanation for how the function works and your thinking behind it. Sorry, I missed the comment in the documentation about the naming the .Rmd files - you're right, it's there and it's clear!

Also, I wanted to thank you for engaging in this discussion with me about what essentially amounts to a question of simplicity vs flexibility. It's a delicate trade-off and it's not obvious that there are right or wrong answers here. It's going to be an issue not just in this function but for the package as a whole, still in its very early stages of development, so having someone to bat these ideas around with is really helpful, and allows me to see how the package is used by someone other than myself.

I think my comments below might get quite long, so please bear with me 😄

I suppose when it comes to naming templates, I can't see who someone would want, say, templates/my-template/template.Rmd when they could have templates/my-template.Rmd, but I concede that could be a lack of imagination on my part, because I recognise that it's important to you, and therefore, presumably for other {distilltools} users.

But the issue I have with that file structure, i.e. using subdirectories and taking template names from the folder name rather than the .Rmd file, is that it sets up an inconsistency between create_post_from_template() and available_templates() about what a template actually is, in a way that could be confusing for users. In create_post_from_template(), a template is specifically an .Rmd file, and the function essentially works by copying that .Rmd file from the path provided to the directory for the post (or talk or whatever), modifying it slightly according to other arguments to the function.

However, in available_templates() as it stands, a template is a subdirectory of the templates folder that contains an .Rmd file somewhere in it, possibly deeply nested.

Having individual templates as subdirectories of a templates folder absolutely makes sense if there was any reason for a template to contain anything in addition to the template .Rmd file. But I'm going to argue that there isn't, so allowing that functionality is extraneous, and might encourage people to do something that serves no purpose (and might even fool them into thinking it does).

create_post_from_template() only looks for and copies an .Rmd file into the directory for the new post. So if there's anything else in the, say, my_template subdirectory containing that .Rmd file, it will be ignored by the function. But that could risk confusing a user who, say, bundled a .css file with the .Rmd as part of the template and expected it to be copied. That could be documented, of course, but it's overly complicated.

I think that the way that .Rmd templates are created in regular R packages is throwing up a bit of a red herring. In that case, it is possible, and often necessary, to bundle supporting files with the skeleton.Rmd file, because packages are designed to be shared with others. So, for example, in a PhD package I wrote that I share with my supervisors, I have an .Rmd template for writing a report that gets bundled with a .bib, a .csl and a .css file.

However, I'd argue that in the context of distilltools, we'd want to explicitly discourage that. It's never necessary because create_post_from_template is always going to be called in the context of a distill website project, and any additional files that are needed for the template to run are always better off placed somewhere else in that directory.

For example, I used a custom syntax highlighting theme on my website, and I have that file in my root directory, then in my ek-post-template.Rmd file, I include the following, full, path, in the yaml:

output:
  distill::distill_article:
    highlight: /Users/ellakaye/ellakaye-distill/ek_syntax_highlighting.theme

I'd argue that that is far preferable to bundling ek_syntax_highlighting.theme in with the template and copying it into each post directory. Firstly, that would create multiple copies of the same file within the same root directory, which is an unnecessary waste of space. Secondly, if I choose to edit my theme, then I only have to edit it in one place and re-knit my posts, rather than changing each copy of the file, and thirdly, we don't have to worry about anything breaking if I change the path to the one copy of the .theme file because posts are not re-knit when building the site.

For all those reasons, it was a deliberate decision to only have create_post_from_template() look for and copy an .Rmd file, and not a template subdirectory with other support files.

So, just as somewhere in the root directory of a distill site, you might have a directory called css that contains just a bunch of .css files and an html directory that contains just a bunch of .html files (as indeed you do for TidyTales!), so it makes sense to have a directory called templates that just contains a bunch of .Rmd files.

You say that you don't want to restrict users or over-complicate the function. What I'm going to propose I think does neither (well, maybe the former, a smidge), and in fact would considerably simplify the function. I suggest removing the functionality to support subdirectories because, as I hope I have convinced, they are not only unnecessary but potentially confusing and inconsistent with how create_post_from_template() defines templates and works. Then you only need to to look in the default templates directory and return as the available templates the .Rmd files (which will be uniquely named) within it. This not only simplifies the function, but also the documentation.

That might restrict users in that it's very opinionated about file structure, but it doesn't restrict them at all in terms of functionality. In fact, I prefer to think of it as helpfully containing them rather than restricting them! You would lose, I'm afraid, the ability to have generic names for the template .Rmd files, but (at least in my opinion) that's a small price to pay for this simpler approach (but, that's easy for me to say, because I already admitted I couldn't see why you'd want that). And, I do appreciate that it might be galling that I'm suggesting that you remove functionality that you've clearly put a great deal of thought and effort into supporting.

Anyway, thank you for bearing with me through all that! I hope it makes sense. That's my view on things as they stand, but I may well be missing something. You say that supporting subdirectories is an important feature that you want to keep in. Why? What use case do you see for subdirectories (other than allowing generic names for names for the template .Rmd files)? Why do you think that allowing generic names is important? I am genuinely curious and worried that I'm missing something important.

At the moment, I think it's more important for create_post_from_template() and available_templates() to be consistent about what a template is than to allow for generic names and subdirectoies, but I'm open to arguments otherwise.

I really look forward to hearing your thoughts on the above. I know it's a lot, so no worries if you want to take some time to think about it!

@mccarthy-m-g
Copy link
Contributor Author

Done, done, and done 😎 We're definitely almost there! And likewise, it's been a pleasure working with you too.

One note: Doing [distilltools::create_post_from_template()] in the see also part of the documentation did not seem to establish the link in the pkgdown site for me. This is my first time using pkgdown so I'm not sure if that's normal or not.

And two other small things:

  1. Want me to add available_templates() to the see also in distilltools::create_post_from_template()?
  2. I think line 11 in DESCRIPTION is redundant since Authors@R is already declared on line 5 (but maybe it's not, I'm not sure)

@EllaKaye
Copy link
Owner

Hi Michael,

I think you may need to remove the back-ticks from around the square brackets, or put them inside it. See the 'Links' section of this vignette. I think you may also need to run devtools::document() to generate the links in the documentation before rebuilding the package and pkgdown site. I'm not 100% sure - my documentation workflow often leans towards trial and error!

On your other two points:

  1. Yes, please add available_templates() to see also for distilltools::create_post_from_template()
  2. You're probably right, but I'm not sure either. You could try removing line 11 in DESCRIPTION, then, after you've rebuilt the package and pkgdown site, check that on the pkgdown site the developers are still listed at the bottom of the right-hand column of the homepage. If so, we're good to go!

@mccarthy-m-g
Copy link
Contributor Author

mccarthy-m-g commented Jun 24, 2021

You do need to run devtools::document() for pkgdown to pick up any changes, I believe it parses the .Rd files to build the site so it won't pick up changes in the source code without doing that first. You were right, the surrounding backticks were the issue! (backticks inside the square brackets are okay, although they're semantically meant to represent a topic, not a function).

I added available_templates() to create_post_from_template() and removed the redundant author line (and the pkgdown site looked the same as before after rebuilding). The pkgdown repo's DESCRIPTION also only has the Authors@R tag so I think we're safe. I also fixed some small grammatical things.

One (last?) thing! What do you think about renaming available_templates()? Either to templates() or something like post_templates(). I think either option feels simpler and more in line with the function's purpose and output. I'm leaning towards post_templates() since it's more descriptive than templates and available_templates and puts it in line with the "post" terminology in distill.

edit (knew there was something else!): It would probably be good to mention the RStudio addin in create_post_from_template()’s documentation too and in the short description of it in the README.

@EllaKaye
Copy link
Owner

Thanks Michael,

My comments below got a bit long (again!) so I've tried to organise them. I think it's a lot of text for fairly few tweaks, though!

Missing .Rd and doc files

I know you say that you've fixed the issue of links not working in available_templates(), but they're still not working for me, either in the RStudio help window, or on the pkgdown site. Did you commit all the changed files after running devtools::document and pkgdown::build_site? I'd expect to see some updated .Rd files in the man folder as well as updates in the doc folder, but they weren't included in the above commits. (When I pulled your changes and ran those functions myself, everything worked fine, but to avoid potential git conflicts, I've discarded those local changes and will wait for you to push them). Plus, there'll be more changes to those files after going through the comments below.

available_templates name

I don't actually mind available_templates as a function name, but am open to changing it if you prefer. I'd be OK with templates, though it is a bit vague. I'm not super-keen on post_templates though, even though it's most consistent with the names distill::create_post and distilltools::create_post_from_template. The reason is that those functions don't actually just create posts, they can create entries in other types of collections too (e.g. I have a collection for 'posts' on my website, as well as one for 'talks', and I have 'ek-post-template.Rmd' and 'ek-talk-template.Rmd' as templates for each.) But I can't do anything about the name create_post and I did at least think create_post_from_template should be consistent with it!

There are a few other names that could work. What about template_paths()? Another thing I've been thinking about, which could be relevant both for naming this function and when fleshing out some of the documentation, is the difference between available_templates() and create_post_from_template() in terms of template paths. We've spent a lot of time discussing file structure for available_templates(), but it's only available_templates() and consequently createPostFromTemplateAddin() that are opinionated about this; running create_post_from_template() in the console still lets you specify any path you wish with the template file. So perhaps changing the name available_templates() to something that reflects that difference could work, e.g. templates_for_addin() or template_paths_for_addin() or available_templates_for_addin() or something along those lines?

Despite the number of words I've now dedicated to it, I really don't feel that strongly about this function name. I see it mostly as a helper function for the addin and something that it's unlikely that users are going to call themselves (except maybe if they're debugging). Happy to go with your preference (including post_templates if you still like that best). If you do change it, can you make sure that all mentions of it in the documentation are updated too.

changes to available_templates() documentation

  • I think it's probably worth wrapping the example in dontrun{} (as in the documentation for create_post_for_template). Otherwise, the output from running the function on the pkgdown site looks a bit weird, not nearly as concise as what a user would see running it within a distill website project (#> [1] "/private/var/folders/xm/n2t41ktd6cj0l3ps1mpk9f6r0000gn/T/RtmpltLNrd/temp_libpath16d94e6adbd/distilltools/rmarkdown/templates/default/skeleton/skeleton.Rmd")

  • swap the @Seealso for [create_post_from_template()], [distill::create_post()] (remove extraneous distilltools:: from before create_post_from_template, swap order for function in our package first, then external package)

changes to create_post_from_template() documentation

  • Absolutely yes to your suggestion to update the create_post_from_template() documentation to discuss the addin. Can I suggest the following as the addition to create_post_from_template() docs, in a new paragraph before the list of @params (feel free to tweak if it's not clear):

distilltools provides an RStudio addin for create_post_from_template. It is accessible from the Addins dropdown menu in RStudio, under DISTILLTOOLS. The addin differs from calling create_post_from_template() in the console in how templates are retrieved. When calling the function in the console, the user must specify a path to an .Rmd file, which can be anywhere in the user's file system. In contrast, the addin provides a dropdown menu of .Rmd templates as found by the [available_templates()] function. By default, [available_templates()] looks for a templates subdirectory in the project's root directory (i.e. ./templates, and returns the named paths of the .Rmd files listed directly in that. So, if for example you have a templates directory containing post-template.Rmd and talk-template.Rmd, then post-template and talk-template will show up in the dropdown list for the Templates box in the addin. There is the option for the user to specify a different default directory for templates. See [available_templates()] for more details. Additionally, the dropdown in the addin will always include Default (even if the user has no template directory or .Rmd templates of their own). The Default template matches that from [distill::create_post()], and as such the addin can be used as an addin for that function too.

  • While you're editing the create_post_from_template documentation, could you fix a couple of things for me in the @example, swapping it for create_post_from_template("templates/post-template.Rmd", "post from template") (better to remove dependency on here, also I forgot the required title argument!)

updates to README

Yes, also, to updating the README to mention the addin and available_templates(). For the README, you'll need to edit the README.Rmd file, then render README.md by running devtools::build_readme(). I suggest the following changes under the 'Functionality' section (but, again, feel free to edit):

  • swap 'four functions' to 'five exported functions'
  • Add to the end of the create_post_from_template bullet point: "There is also an RStudio addin to create a post, which can either use the [distill::create_post()] default template or an .Rmd file found (by default) in a templates subdirectory of the project root directory (i.e. ./templates). See [create_post_from_template()] and [available_templates()] and for more details on where to store templates so they appear in the addin Templates dropdown list, and how to change the default templates directory location."

change to addin name?

One other idea I've been ruminating on is changing the name of the addin in the dropdown list to "Create post (including from template)". That's slightly less consistent with the create_post_from_template function name, but better reflects the fact that the addin works just as well for distill::create_post (and I suspect that there are lots of distill users out there who might want to use the addin just with the default without creating their own templates). Again, don't feel too strongly about it either way. What do you think?

@mccarthy-m-g
Copy link
Contributor Author

I figured there would be more changes coming so I didn't commit docs and pkgdown changes last time. I did this time so it's easier for you to look though 😄

available_templates name

I've left it as is for now. I also don't feel too strongly about it since available_templates() serves its purpose. Given that create_post_from_template() can create "posts" in other collections such as talks, I agree a general name for available_templates() is better than one that references posts. Similarly, since it can be used in create_post_from_template() in the console as well I don't think it should reference the addin in the function name.

I'm good keeping it as is. I also thought list_templates() could be an option, but maybe that's too general. The nice thing about available_templates() is that the name implies there are locations templates are available from, and locations where they are not, which reflects how the function actually works.

changes to available_templates() documentation

Done.

changes to create_post_from_template() documentation

Fixes made.

Let me know what you think about the changes I made. I edited the existing documentation to make it less verbose (but hopefully still just as clear), and tried to shorten the paragraph you suggested as well as have it reflect what was already written in available_templates(). In general I think if we have complimentary functions we should try to point users to the documentation of the other function for details rather than repeating specific details in both functions. This will be easier to maintain and avoid mismatching documentation between functions if someone changes one and not the other.

updates to README

Done. I also fixed the header levels throughout the README, and added subheadings under functionality to group the headings. The headings I chose align with the sentence at the start of the README

"distilltools is collection of tools to support the creation and styling of content on websites created using the distill package in R."

change to addin name?

I don't feel too strongly here as long as the name is clear about what the template does. Some thoughts:

  • "Create post from template" is more consistent with the rest of the naming schemes which is a plus. The default template is still a template so this name gets the point across too, but some users might not realize this if they skim through (or don't read) the documentation.
  • "Create post (including from template)" feels a bit clunky but helps imply that it can act just like distill::create_post. Maybe "Create post (from template)" instead? That feels less clunky to me and I would be good with it.
  • "Create post" is another option. If the template argument is added to distill::create_post() this might be the best choice. But until then I think it isn't specific enough since some users might think the addin won't work with templates.

@EllaKaye
Copy link
Owner

Thanks Michael 😄

available_templates() name

Fine by me! Happy to leave as is.

available_templates() documentation

Looking good!

create_post_from_template() documentation

Your concise paragraph gets the job done. It's an improvement on my suggestion and still covers everything that needs saying.

If I'm being nit-picky, I'd suggest making the first mention of available_templates() in the details paragraph into a link. Also, updating the two references to Create post from template to Create post (from template) (see below).

README

Really like your changes here. Thanks for fixing the header levels. As the package expands, totally makes sense to separate out blogging and styling functions.

Just one thing. The link to distill::create_post() in the create_post_from_template() bullet point is broken. It seems that GitHub Flavoured Markdown requires something different to markdown in Roxygen comments to make this work. I think swapping the square brackets for back-ticks should do the trick.

Change to addin name

None of these are perfect, but I reckon that Create post (from template) is probably best balance between consistency with function name and offering an immediate sense of its use for both distill::create_post and create_post_from_template for users who don't read the docs properly (or perhaps the slightly unusual addin name will encourage users to read said docs!)

If you wouldn't mind making those final tweaks, I think this PR will be ready to merge!

@mccarthy-m-g
Copy link
Contributor Author

No problem! Final tweaks made--I think we're good to go! 😄

@EllaKaye EllaKaye merged commit 7040f17 into EllaKaye:main Jun 29, 2021
@EllaKaye
Copy link
Owner

Done! 🎉 🎉 🎉

Thank you SO MUCH, Michael, for absolutely everything on this 👏 👏 👏 . From you initial enthusiasm for {distilltools} to initiating the PR with a pretty much fully-functioning addin, to engaging with me in numerous debates about fine-tuning it, to making what probably seemed like never-ending tweaks at my request, to tidying up the docs, you've been an absolute pleasure to work with on this. I hope we have opportunities to collaborate again in the future 😄. If you have any further ideas for additional {distilltools} functionality (especially as you continue to develop TidyTales), do let me know!

@EllaKaye EllaKaye mentioned this pull request Jun 29, 2021
@mccarthy-m-g
Copy link
Contributor Author

Woohoo! 🎉🎉🎉

You’re very welcome! Thank you for the kind comments 😄 This was my first contribution to another R package and you made it a great experience. It was a pleasure working with you too! So likewise, I hope we can do so again!

I was thinking a wrapper function for Miles McBain’s HTML script to add Utterances comments to distill articles might be a nice addition. Utterances allows themes and some other customizations, wrapping the creation of the script file into a function would make the availability of those customizations more explicit (and might also just make distill users more aware they can even use Utterances instead of Disqus).

@EllaKaye
Copy link
Owner

EllaKaye commented Jul 1, 2021

I hadn't realised it was your first package contribution. What an absolutely stellar way to start! No doubt not your last, at least hopefully not for distilltools, and I'm sure you'll continue to make your mark elsewhere across the R ecosystem.

I really like your idea for the utterances function 😄 I have utterances enabled on my blog but hadn't realised it was customisable (at least not until I dug into your TidyTales templates and css 😎!)

Have you seen this blogpost on enabling Utterances on distill blogs? I think it's been pretty widely shared and most distill blogs I've seen that have initiated comments have used Utterances rather than Disqus, but I think the function you're proposing would certainly be a great way to keep making a point for the preferability of the former over the latter!

It would also be a different kind of function for distilltools and so a nice test case for thinking about how it will best work from a user experience point of view, e.g. where the script gets saved, do we need to write any css anywhere and if so what kind of assumptions might we make about what the user already has (e.g. whether they've run distill::create_theme()), what we automate and what we leave as instructions for the user (e.g. like some usethis and blogdown functions provide a console print out of what running the function has done and what's left for the user to edit). We could bat some ideas around in an issue before you submit a PR, or else you could kick off a PR with something and we can take it from there.

I might be a little slower to respond to things for a while - lots of work and family commitments at the moment. But I'm certainly happy to take a look at whatever you have in mind as and when I get the chance and I'm sure we'll get there eventually!

@mccarthy-m-g
Copy link
Contributor Author

Thanks, I appreciate it! 😄

I believe that's the blog I followed when searching how to add Utterances! So you're probably right it's been widely shared and is easy to find. But convenience is always nice!

I'll open an issue where we can continue our conversation. No worries about slower responses, I need to focus on work for my Master's thesis right now so I'll be a bit busier too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants