Allow full url in `theme` arg in `install_theme` (#270) #271

gadenbuie · 2018-03-06T17:02:08Z

This PR allows the theme argument in new_site() and install_theme() to accept a full URL to the zip file of the theme, in line with the discussions in #264 and #270.

Some notes:

I did not add any validation on the URL other than to check that it ends in .zip. This lets the process be as flexible as possible and download2() will throw the error if the URL is invalid or doesn't work. This also means the user can specify a local file, like file:///home/user/archive.zip or any other unusual configuration.

Because I didn't attempt to guess at the structure of the URL, we don't know the theme's base name and we have to rely on the first level folder in the zip file. This is in line with how install_theme() currently works, but previously you used knowledge of the branch (parsed from the theme argument) to remove the trailing -<branch> from the theme name. The end result here is that the theme may end up with a long, unique name depending on how it's zipped by the repo host, but this will be transparent to the user (no warning or message emitted).

To illustrate the process, in the use case described in #270,

the full URL is https://gitlab.com/syui/hugo-theme-wave/repository/master/archive.zip,
the theme is downloaded, extracted and stored in themes/hugo-theme-wave-master-13b859a257213881038ebeb1021a5578c243653b
and the config file is changed to theme = "hugo-theme-wave-master-13b859a257213881038ebeb1021a5578c243653b"

Finally, there is the question of force = TRUE. Sticking to minimal changes, specifying the full URL is equivalent to force = TRUE because the folder name is only changed using the user/repo style name. Previously, you were reasonably certain that the zip file would extract into a different folder than the final folder, so there aren't any checks that the unzipping will overwrite. This is the expected behavior in d198998.

I added a second commit, 4bda539, where the zip file is extracted into a temp folder inside themes/. The tmpdir name is stripped from the zip file list so that we still get the theme folder, and if force = FALSE and the theme dir (zipdir) exists inside themes/ the error is thrown, tmpdir is unlinked, and the zip file remains (as this was the previous behavior). I've separated this into a second commit so that you can see behavior with and without the usage of a tmpdir. Feel free to revert or ask for changes as needed.

p.s. will be happy to add news entry and finalize PR, etc. after your review.

gadenbuie · 2018-03-06T17:13:12Z

FYI here are three example URLs

github_url = "https://github.com/road2stat/hugo-tanka/archive/master.zip"

gitlab_url = "https://gitlab.com/syui/hugo-theme-wave/repository/master/archive.zip"

bitbucket_url = "https://bitbucket.org/thunderrabbit/hugo-theme-thunderrabbit/get/master.zip"

yihui · 2018-03-06T19:45:39Z

R/hugo.R

    zipdir = dirname(files)
+    zipdir = gsub(tmpdir, ".", zipdir)


I think this is too early. The dir expdir on line 173 won't be found if you strip tmpdir from zipdir at this point.

Good catch! I just moved line 170 down so that it's stripped after finding expdir.

Actually, on second thought, it's only needed for newdir, so I'm going to refactor a bit.

yihui

Okay, looks good to me now.

For newdir, do you think it make sense to remove -[a-f0-9]{32,}$ (which "apparently" looks like SHA)?

yihui · 2018-03-06T20:22:37Z

Please also add yourself to DESCRIPTION as ctb (alphabetical order by first names) and an item in NEWS.md. Thanks!

gadenbuie · 2018-03-06T20:22:46Z

R/hugo.R

@@ -179,15 +178,16 @@ install_theme = function(
      'and at least take a look at the config file config.toml of the example site, ',
      'because not all Hugo themes work with any config files.'
    )
-    newdir = gsub(sprintf('-%s$', branch), '', zipdir)
+    newdir = gsub(tmpdir, ".", zipdir)
+    newdir = gsub(sprintf('-%s$', branch), '', newdir)


@yihui Should I add an if (!theme_is_url) here so that it doesn't even try to change the filename if the zip file is given? The alternative would be to add some logic around the branch regex and then hope we get lucky at this stage ... so seems to me to be better to skip this if theme_is_url. I didn't do this because I was trying not to add a ton of logic and branching paths, but it's been nagging at me.

Conditioning on theme_is_url makes sense to me.

gadenbuie · 2018-03-06T20:29:05Z

Re SHA, that's something I played with too. Getting rid of it is easy with that regex, it's easier than guessing the branch. But maybe it won't work depending on how the repo is bundled? So I kinda settled on consistency in the sense that the theme dir is the directory given by the bundler (see my comment on removing branch)... that said I'm happy to add commit hashes removal if you think it's a good idea.

gadenbuie · 2018-03-06T20:38:16Z

Here's a quick sketch for trying to clean up the theme directory name. If the theme is a URL, guess that branch = 'master'.

if (branch == '' | theme_is_url) branch = 'master'

Then remove SHA hash if at end of directory name, and then remove branch name if at the end.

newdir = gsub(tmpdir, ".", zipdir)
newdir = gsub("-[a-f0-9]{7,40}$", "", newdir)
newdir = gsub(sprintf('-%s$', branch), '', newdir)

Note the regex is from https://stackoverflow.com/a/468378/2022615 -- bitbucket uses shortened hashes, like thunderrabbit-hugo-theme-thunderrabbit-c418b1a1584d.

To make this easy, give this a thumbs up and I'll push above changes. thumbs down is don't change folder name at all if theme is a URL.

yihui · 2018-03-06T21:00:07Z

👍 I feel 5 is probably a little risky... Removing the SHA hash is a feature "good to have", but not really necessary. For now, we can probably just stick with 40, and change it when other users actually request it.

yihui · 2018-03-06T21:08:35Z

R/hugo.R

@@ -151,7 +151,7 @@ install_theme = function(
    return(invisible())
  }
  branch = sub('^@', '', gsub(r, '\\2', theme))
-  if (branch == '') branch = 'master'
+  if (branch == '' | theme_is_url) branch = 'master'


| -> ||

gadenbuie · 2018-03-06T21:10:30Z

Cool and I agree! I kinda went middle of the road and set lower limit to 12 so that bitbucket zips work. There's no chance for clashes with any English words at that limit. Using branch = "master" when the theme_is_url works really well for GitHub and Gitlab.

yihui · 2018-03-06T21:35:35Z

Perfect. Thanks @gadenbuie!

yihui · 2018-03-06T21:37:06Z

Actually could you do me one more favor to reply to #270?

gadenbuie · 2018-03-06T21:44:46Z

Of course, thanks for the reminder!

yihui · 2018-03-11T20:56:11Z

This PR probably introduced a bug on Windows: https://stackoverflow.com/q/49212335/559676 Do you mind following up with that post and see if you are able to fix it? @gadenbuie (Unfortunately I've been sick for a few days)

gadenbuie · 2018-03-12T00:04:06Z

@yihui Sure thing, I'll take a look tonight. Sorry about the bug, Windows is a bit of a blind spot for me. And hope you feel better soon!

gadenbuie · 2018-03-12T01:23:56Z

@yihui I was able to reproduce the issue, which I believe I've solved with #276. I replied in a comment to the SO question, pointing them to the new PR and with instructions to get back to previous behavior in the meantime. Let me know if there's anything else I can do.

gadenbuie added 2 commits March 6, 2018 11:10

Allow full URL in theme arg in new_site and install_theme (rstudio#270)

d198998

Unzip theme into tmpdir to respect force arg

5b52575

yihui requested changes Mar 6, 2018

View reviewed changes

gadenbuie added 2 commits March 6, 2018 15:00

Strip tmpdir from zipdir after finding expdir

0313491

Refactor zipdir/newdir in install_theme

d965687

yihui approved these changes Mar 6, 2018

View reviewed changes

gadenbuie commented Mar 6, 2018

View reviewed changes

yihui added this to the v0.6 milestone Mar 6, 2018

Clean SHA and branch from theme install dir

b0034f1

yihui reviewed Mar 6, 2018

View reviewed changes

gadenbuie added 2 commits March 6, 2018 16:11

Hard OR if branch is empty string

66d18ed

Add contributor and news item for theme arg update

cf64322

yihui merged commit 4266c33 into rstudio:master Mar 6, 2018

yihui added a commit that referenced this pull request Mar 7, 2018

roxygenize after #271

af35e49

gadenbuie deleted the theme-full-url branch March 9, 2018 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow full url in `theme` arg in `install_theme` (#270) #271

Allow full url in `theme` arg in `install_theme` (#270) #271

gadenbuie commented Mar 6, 2018 •

edited

gadenbuie commented Mar 6, 2018

yihui Mar 6, 2018

gadenbuie Mar 6, 2018

gadenbuie Mar 6, 2018

yihui left a comment

yihui commented Mar 6, 2018

gadenbuie Mar 6, 2018

yihui Mar 6, 2018

gadenbuie commented Mar 6, 2018

gadenbuie commented Mar 6, 2018 •

edited

yihui commented Mar 6, 2018

yihui Mar 6, 2018

gadenbuie commented Mar 6, 2018

yihui commented Mar 6, 2018

yihui commented Mar 6, 2018

gadenbuie commented Mar 6, 2018

yihui commented Mar 11, 2018

gadenbuie commented Mar 12, 2018

gadenbuie commented Mar 12, 2018

Allow full url in theme arg in install_theme (#270) #271

Allow full url in theme arg in install_theme (#270) #271

Conversation

gadenbuie commented Mar 6, 2018 • edited

gadenbuie commented Mar 6, 2018

yihui Mar 6, 2018

Choose a reason for hiding this comment

gadenbuie Mar 6, 2018

Choose a reason for hiding this comment

gadenbuie Mar 6, 2018

Choose a reason for hiding this comment

yihui left a comment

Choose a reason for hiding this comment

yihui commented Mar 6, 2018

gadenbuie Mar 6, 2018

Choose a reason for hiding this comment

yihui Mar 6, 2018

Choose a reason for hiding this comment

gadenbuie commented Mar 6, 2018

gadenbuie commented Mar 6, 2018 • edited

yihui commented Mar 6, 2018

yihui Mar 6, 2018

Choose a reason for hiding this comment

gadenbuie commented Mar 6, 2018

yihui commented Mar 6, 2018

yihui commented Mar 6, 2018

gadenbuie commented Mar 6, 2018

yihui commented Mar 11, 2018

gadenbuie commented Mar 12, 2018

gadenbuie commented Mar 12, 2018

Allow full url in `theme` arg in `install_theme` (#270) #271

Allow full url in `theme` arg in `install_theme` (#270) #271

gadenbuie commented Mar 6, 2018 •

edited

gadenbuie commented Mar 6, 2018 •

edited