start functionalizing make screenshots #153

kweav · 2024-07-08T19:13:22Z

This PR begins implementing #145 by functionalizing make_screenshots.R, adding it to ottrpal.

The approach I took was creating params based on the optparse options info in the script. I also carried over the functionality of the script. I'll leave a few comments with questions. Later stacked PRs will set up tests and update the base_ottr image and update render_all.yml as described in #145

kweav · 2024-07-08T19:38:46Z

R/screenshot.R

+#' @param git_pat default is NULL; required argument; a Git secret
+#' @param repo default is NULL; required argument; GitHub repository name, e.g., jhudsl/OTTR_Template
+#' @param output_dir default is "resources/chapt_screen_images"; Output directory where the chapter's screen images should be stored
+#' @param base_url default is NULL; rendered bookdown URL where screenshots are taken from, if NULL, the function will use the repo_name and and git_pat to find the base_url


Had to edit this description to change it from the original in the script. How is it?

Looks like all the essentials are here! Only thing you could add if you wanted to be extra nice is a link to the page in GitHub about PATs: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens

kweav · 2024-07-08T19:39:10Z

R/screenshot.R

+#' @param git_pat default is NULL; required argument; a Git secret
+#' @param repo default is NULL; required argument; GitHub repository name, e.g., jhudsl/OTTR_Template


What's the best way to communicate that these are required arguments, but by default NULL?

I would just not have them be NULL if they are required because then it will automatically throw an error if no argument is provided. And docs wise the way you say its "required" is fine 👍

kweav · 2024-07-08T19:39:27Z

R/screenshot.R

+  }
+
+  if (is.null(base_url)){
+    base_url <- cow::get_pages_url(repo_name = repo, git_pat = git_pat) #what if these arguments are still NULL/not supplied?


Do we want to make this cow function part of ottrpal?

Yes! That's next up! It's mentioned in #137 but I didn't realize I didn't make it its own issue so there is one now: jhudsl/cow#27

kweav · 2024-07-08T19:39:40Z

R/screenshot.R

+
+  message(paste("Image Chapter key written to: ", file.path(output_folder, "chapter_urls.tsv")))
+
+}


Do I need to return something from this function?

I think returning the file path to chapter_urls.tsv makes sense.

kweav · 2024-07-08T19:40:13Z

pkgdown is failing because of trying to import cow as expected (and slacked)

cansavvy

This is looking good! 🎉 Some minor comments!

Is the unit test for this coming in a next PR?

cansavvy · 2024-07-09T15:33:18Z

R/screenshot.R

+#' @param git_pat default is NULL; required argument; a Git secret
+#' @param repo default is NULL; required argument; GitHub repository name, e.g., jhudsl/OTTR_Template


I would just not have them be NULL if they are required because then it will automatically throw an error if no argument is provided. And docs wise the way you say its "required" is fine 👍

cansavvy · 2024-07-09T15:36:47Z

R/screenshot.R

+#' @param git_pat default is NULL; required argument; a Git secret
+#' @param repo default is NULL; required argument; GitHub repository name, e.g., jhudsl/OTTR_Template
+#' @param output_dir default is "resources/chapt_screen_images"; Output directory where the chapter's screen images should be stored
+#' @param base_url default is NULL; rendered bookdown URL where screenshots are taken from, if NULL, the function will use the repo_name and and git_pat to find the base_url


Looks like all the essentials are here! Only thing you could add if you wanted to be extra nice is a link to the page in GitHub about PATs: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/managing-your-personal-access-tokens

R/screenshot.R

cansavvy · 2024-07-09T15:41:51Z

R/screenshot.R

+  }
+
+  if (is.null(base_url)){
+    base_url <- cow::get_pages_url(repo_name = repo, git_pat = git_pat) #what if these arguments are still NULL/not supplied?


Yes! That's next up! It's mentioned in #137 but I didn't realize I didn't make it its own issue so there is one now: jhudsl/cow#27

cansavvy · 2024-07-09T15:42:22Z

R/screenshot.R

+
+  message(paste("Image Chapter key written to: ", file.path(output_folder, "chapter_urls.tsv")))
+
+}


I think returning the file path to chapter_urls.tsv makes sense.

kweav · 2024-07-09T15:50:35Z

This is looking good! 🎉 Some minor comments!

Thanks! And thanks for the comments and suggestions! They're very helpful

Is the unit test for this coming in a next PR?

Yes, the unit test will be the next PR. Can you point me to any examples/guides you want me to follow for creating a good unit test?

Co-authored-by: Candace Savonen <cansav09@gmail.com>

kweav · 2024-07-09T15:56:15Z

For this PR my next steps will include addressing the suggestions/feedback here
And I will add a stacked PR that incorporates a unit test
And then I can also try to move the cow function into ottrpal in another stacked PR if you'd like?

kweav · 2024-07-09T17:42:03Z

Latest commit addressed

adding the git pat link to the docs
removing NULL as a default from the two required arguments
tried to add an example of running the function (is that a good way of showing getting the secret?)
also have it return the file path now

copy over cow function to ottrpal

kweav · 2024-07-09T18:44:38Z

@cansavvy after merging #154 and removing the cow dependency, I was hoping that pkgdown wouldn't fail. But now it's failing because there is no ‘webshot2’ package

Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) : 
  there is no package called ‘webshot2’

kweav · 2024-07-10T18:35:45Z

@cansavvy after merging #154 and removing the cow dependency, I was hoping that pkgdown wouldn't fail. But now it's failing because there is no ‘webshot2’ package
Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) : 
  there is no package called ‘webshot2’

We updated the base_ottr image in jhudsl/ottr-docker#27 to include webshot2 and its various dependencies

kweav · 2024-07-10T21:05:30Z

@cansavvy Given the R-CMD-check warnings about check_git_repo get_git_auth and get_repo_info not being global functions, I need to bring over those cow functions to ottrpal sooner rather than later, right? I know check_git_repo was next on my list, but should probably add the other two as well, right?

cansavvy · 2024-07-11T14:39:03Z

@cansavvy Given the R-CMD-check warnings about check_git_repo get_git_auth and get_repo_info not being global functions, I need to bring over those cow functions to ottrpal sooner rather than later, right? I know check_git_repo was next on my list, but should probably add the other two as well, right?

Yes this needs to be done. Probably transferring these at the same time makes sense. But probably in a next PR. 👍

cansavvy · 2024-07-19T16:11:40Z

Fixed the description file so now this is ready to merge!

Next PR can include more unit tests for this!

kweav added 2 commits July 8, 2024 15:03

start functionalizing make screenshots

62c8ae6

Merge branch 'main' into ki/refactor

af2d370

kweav commented Jul 8, 2024

View reviewed changes

kweav requested a review from cansavvy July 8, 2024 19:39

cansavvy approved these changes Jul 9, 2024

View reviewed changes

Update R/screenshot.R

23f0fa5

Co-authored-by: Candace Savonen <cansav09@gmail.com>

address quick suggestions

0aabec8

copy over cow function to ottrpal

4f85a8c

kweav mentioned this pull request Jul 9, 2024

copy over cow function to ottrpal #154

Merged

kweav added 2 commits July 9, 2024 14:36

update example for get pages and add export to screenshot

edddb94

Merge pull request #154 from jhudsl/ki/refactor-cowfn

0a38e72

copy over cow function to ottrpal

cansavvy and others added 2 commits July 10, 2024 14:45

Update R-CMD-check.yaml

8dd642d

Update DESCRIPTION

d8f1622

Fix description file for cow dependency

55005af

cansavvy merged commit 3d56b9f into main Jul 19, 2024
2 checks passed

cansavvy deleted the ki/refactor branch July 19, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

start functionalizing make screenshots #153

start functionalizing make screenshots #153

kweav commented Jul 8, 2024

kweav Jul 8, 2024

cansavvy Jul 9, 2024

kweav Jul 8, 2024

cansavvy Jul 9, 2024

kweav Jul 8, 2024

cansavvy Jul 9, 2024

kweav Jul 8, 2024

cansavvy Jul 9, 2024

kweav commented Jul 8, 2024

cansavvy left a comment

cansavvy Jul 9, 2024

cansavvy Jul 9, 2024

cansavvy Jul 9, 2024

cansavvy Jul 9, 2024

kweav commented Jul 9, 2024

kweav commented Jul 9, 2024

kweav commented Jul 9, 2024

kweav commented Jul 9, 2024

kweav commented Jul 10, 2024

kweav commented Jul 10, 2024

cansavvy commented Jul 11, 2024

cansavvy commented Jul 19, 2024

		#' @param git_pat default is NULL; required argument; a Git secret
		#' @param repo default is NULL; required argument; GitHub repository name, e.g., jhudsl/OTTR_Template


		message(paste("Image Chapter key written to: ", file.path(output_folder, "chapter_urls.tsv")))

		}

start functionalizing make screenshots #153

start functionalizing make screenshots #153

Conversation

kweav commented Jul 8, 2024

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

kweav commented Jul 8, 2024

cansavvy left a 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

kweav commented Jul 9, 2024

kweav commented Jul 9, 2024

kweav commented Jul 9, 2024

kweav commented Jul 9, 2024

kweav commented Jul 10, 2024

kweav commented Jul 10, 2024

cansavvy commented Jul 11, 2024

cansavvy commented Jul 19, 2024