-
-
Notifications
You must be signed in to change notification settings - Fork 334
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
Resolve issues about archetypes and new posts with a non-default kind
#263
Conversation
…arkdown. Resolves #261. Details: new_content() will create a copy of the file with a .md termination so that hugo will be able to work with it. Once hugo is done, the temporary files are deleted.
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.
Next time if you are going to do three orthogonal things, I recommend you to submit three pull requests. In this case, you could have submitted three "bite-sized" PRs, and I could happily accept each in 5 minutes. If all three come in the same PR, I cannot accept the obviously good changes/fixes because of the less ideal ones.
This time you are fine. No need to split this PR, but I'll need more time to review it (looks good overall). Thanks!
@@ -1,7 +1,7 @@ | |||
Package: blogdown | |||
Type: Package | |||
Title: Create Blogs and Websites with R Markdown | |||
Version: 0.5.4 | |||
Version: 0.5.6 | |||
Authors@R: c( |
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.
Please also add yourself as a ctb
here (alphabetical order by first name).
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.
done ^^
Ok, I'll keep this in mind! Though from my point of view the 3 changes were related. In any case, I understand where you are coming from. |
inst/scripts/new_post.R
Outdated
) | ||
shiny::stopApp() | ||
}) | ||
shiny::observeEvent(input$cancel, { | ||
shiny::stopApp() | ||
}) | ||
}, | ||
stopOnCancel = FALSE, viewer = shiny::dialogViewer('New Post', height = 500) | ||
stopOnCancel = FALSE, viewer = shiny::dialogViewer('New Post', height = 570) |
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.
Could you try to move the Archetype
menu to the right of Tags
and see how it looks like? I prefer not to increase the height of this addin if possible.
R/hugo.R
Outdated
if (!file.exists(file.path('archetypes', atype))) return('default') | ||
gsub('/.*', '', path) | ||
## Assumes path is something like | ||
## /pathToBlogdownDir/content/post/2018-02-24-postslug.Rmd |
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.
path
is supposed to be a relative path without the leading content/
; new_content('foo/bar.md')
means to create content/foo/bar.md
. Does that make sense?
R/hugo.R
Outdated
hugo_cmd(c('new', shQuote(path), c('-k', kind))) | ||
file = content_file(path) | ||
hugo_toYAML(file) | ||
} |
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.
I feel this is too complicated. Will this work (no need if-else
)?
# create a .md file and rename it to the expected extension later
path2 = with_ext(path, '.md')
file = content_file(path)
file2 = content_file(path2)
hugo_cmd(c('new', shQuote(path2), c('-k', kind)))
hugo_toYAML(file2)
file.rename(file2, file)
…ded in case that the file does indeed have a .md file extension (otherwise it gets deleted). Hopefully this look less complicated
I made 3 commits each addressing one of your requested changes. |
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.
Could you also provide a screenshot of the new addin? I want to make sure the spacing looks correct. Thanks!
R/hugo.R
Outdated
## If we do this after fixing the file termination it doesn't work | ||
hugo_toYAML(file_temp) | ||
file = gsub('md$', tools::file_ext(path), file_temp) | ||
file.copy(file_temp, file) |
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.
Basically I had written the full code in the comment (meant to replace your lines 193-206 above): #263 (comment) Did you test it? (I was asking if that would work)
# create a .md file and rename it to the expected extension later
path2 = with_ext(path, '.md')
file = content_file(path)
file2 = content_file(path2)
hugo_cmd(c('new', shQuote(path2), c('-k', kind)))
hugo_toYAML(file2)
file.rename(file2, file)
R/hugo.R
Outdated
## (file termination doesn't matter) | ||
atype = gsub('.*/', '', dirname(path)) | ||
if (!file.exists(file.path('archetypes', paste0(atype, '.md')))) return('default') | ||
atype |
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.
With the assumption that path
is a relative path like post/2018-02-24-postslug.Rmd
, I don't see the need for any changes in this function: gsub('/.*', '.md', path)
will return post.md
.
… correctly that path is post/date-slug.md or something like that instead of content/post/date-slug.md
Hi, Here's the screenshot you requested (it looks better imo): Having understood the default_kind = function(path) {
path = normalizePath(path, '/', mustWork = FALSE)
if (!grepl('/', path)) return('default')
atype = dirname(path)
if (!file.exists(file.path('archetypes', with_ext(atype, '.md')))) return('default')
atype
} But it does exactly the same thing as the version you had: default_kind = function(path) {
path = normalizePath(path, '/', mustWork = FALSE)
if (!grepl('/', path)) return('default')
atype = gsub('/.*', '.md', path)
if (!file.exists(file.path('archetypes', atype))) return('default')
gsub('/.*', '', path)
} I also missed the code in the comments and it does indeed work, so I'm using your code there 7b18d1d. I tested it with both a Thanks for your patience! I'm new to PRs despite using R and GitHub for years now :P Will GitHub automatically squash the history? Or do I have to run something to make it cleaner? Best, |
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.
Don't worry about GIT: I can choose to squash the commits on Github (as I always do). Thanks!
Cool, thanks! |
Hi,
This pull request resolves two related issues.
It addresses #173 (comment) so that
default_kind()
can find archetypes such asarchetypes/post.md
for files like/content/post/2018-02-24-postslug.whatever
. It also adds the option to choose the archetype from the RStudio addin as described in #173 (comment).Next, it solves the situation described in #261 (comment) where
new_content()
wouldn't work with a providedkind
(archetype) if the file did not end in.md
.Screenshots of PR working
Here are two screenshots showing it all working together now. First, I use the modified RStudio addin to select the
post
archetype (which you can see in the background, contents posted later here also).Next is a screenshot of the resulting
.Rmd
new post file wherehugo_toYAML()
added the appropriate slug, categories and tags + the rest of the contents from thepost.md
archetype (aka,kind = 'post'
).archetypes/post.md
archetype initial contentsLimitations / stuff for the future
kind
has a non.md
file termination in this PR. I realize now that this is a hugo property. In any case, I'm not sure in which of the blogdown functions such a warning would go.hugo_toYAML()
will keep/add this part to the YAML section:Misc
NEWS.md
file following the syntax used previously in the file. Please feel free to edit it. For example, maybe you prefer linking to this PR instead of the issues.new_content()
I usetools::file_ext()
which should be ok, since you havetools
undersuggests
in theDESCRIPTION
file.Best,
Leo