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

simplest way to add the embedded files and video scripts although possibly there is a better way #3

Closed
wants to merge 1 commit into from

Conversation

carriewright11
Copy link
Member

No description provided.

Copy link
Contributor

@cansavvy cansavvy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally we would split up the code here into two things:

  1. A script that can be called by the user
  2. Functions that are automatically included into the package

I think the items for 1) should be instead in the _Leanpub template as their own script. This script would also be called by a github actions in that repository so it would automatically be run.
The items for 2) would remain in this repository and be integrated into this package like you've done here.

Does this sound good/make sense?

I propose we don't merge this quite yet and I can work on splitting up 1 and 2 while I test out the things you have here. Then I can file PRs for 1 and 2 and request your review so you can check if my understanding is correct.

# script to fix embedded files for Leanpub
# originally used the raw rmd files before conversion with bookdown_to_leanpub() - now part of the function at the beginning

library(here)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since you added these in the dependencies list, having them called by library() here is not necessary.

files<-list.files(here(), pattern = ".rmd|.Rmd|.RMd|.RMD")

#code to modify the lines within the rmd about the embedded files - to instead create a link
for(i in files){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally this would be a function too. I can work on functional-izing this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be used for a function to add text about feedback...

# script to fix videos for Leanpub
# originally used the output of the bookdown_to_leanpub() - now part of the function at the end

library(here)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here.

@cansavvy
Copy link
Contributor

And of course after we make the changes from the above items, if these checks are still failing, we should re-assess and dig into that.

for(i in files){
tx_i <- readLines(here::here(i))
# want lines that have include_url but not youtube
to_replace_embed_knitr <-tx_i[grepl(pattern = "include_url", tx_i, fixed=TRUE) & !grepl("^<!--", trimws(tx_i)) & !grepl("youtube", trimws(tx_i))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense for this to be its own function in the package.

@carriewright11
Copy link
Member Author

And of course after we make the changes from the above items, if these checks are still failing, we should re-assess and dig into that.

I believe the checks are already failing

@carriewright11
Copy link
Member Author

I think ideally we would split up the code here into two things:

  1. A script that can be called by the user
  2. Functions that are automatically included into the package

I think the items for 1) should be instead in the _Leanpub template as their own script. This script would also be called by a github actions in that repository so it would automatically be run.
The items for 2) would remain in this repository and be integrated into this package like you've done here.

Does this sound good/make sense?

I propose we don't merge this quite yet and I can work on splitting up 1 and 2 while I test out the things you have here. Then I can file PRs for 1 and 2 and request your review so you can check if my understanding is correct.

Just to clarify... do you mean by "split up the code" so that part of the current scripts would be in the leanbuild package and part would be in scripts in the leanpub template? couldn't we just call through a git action (with a simple script) an updated version of the package::bookdown_to_leanpub() after it incorporates all the code from my scripts and does everything in one step? Or maybe you did not want to not include some of the conversions? We could include arguments about running specific conversions if that was the case.

@cansavvy
Copy link
Contributor

I think this PR has served its purpose. These changes have been integrated to the package on #14. Closing this PR now.

@cansavvy cansavvy closed this Aug 26, 2021
@cansavvy cansavvy deleted the video_and_embedded branch September 28, 2021 16:49
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