Skip to content

Conversation

@acoffman
Copy link
Contributor

Purpose/implementation Section

Talking to @kweav, It was decided that the borrow chapter functionality should be limited to public repos only. This means there does not need to be a token present for the api call. We also decided it would be best to refactor the method to use the GitHub API like the rest of the methods in this file.

However, the check_git_repo function would fail if a token was not provided as it falls back on get_token(app_name = "github" which itself errors if authorize("github") has not previously been called.

What changes are being implemented in this Pull Request

  • Remove the token parameter from the borrow chapter implementation
  • Refactor the check_git_repo function to still allow a token, but not fail if it is not provided with one
  • Refactor the check_git_repo function to utilize the GitHub api rather than git remote-ls
  • Clean up two places where the incorrect authorization header was being sent (not directly related this PR)

A couple of other notes:

  • The previous implementation would fail if the remote was added via git@ rather than https
  • I checked both the repos here and the public GitHub search and was unable to find anything utilizing the return_repo result file that we lose as part of moving away from git remote-ls

What GitHub issue does your pull request address?

N/A

Tell potential reviewers what kind of feedback you are soliciting.

Any! This was tested as follows:

remove.packages("ottrpal")
library(devtools)
install_local("~/git/ottrpal")

ottrpal::check_git_repo(repo_name = "jhudsl/Reproducibility_in_Cancer_Informatics")
TRUE
ottrpal::check_git_repo(repo_name = "jhudsl/Reproducibility_in_Cancer_Informatic")
FALSE

@acoffman acoffman changed the title Acoffman/borrow chapter token fix Borrow chapter token fix Dec 15, 2025
Change file reading from doc_path to dest_file before processing to attempt to fix error identified while testing this branch
@kweav
Copy link
Contributor

kweav commented Dec 15, 2025

@acoffman this looks great -- thank you! I stacked a PR (#20) so that the full function will work, and I've got some warnings I didn't expect even though rendering is working. But probably will be merging this soon once I figure those out!

…er-fix

Update file reading for content modification
@kweav
Copy link
Contributor

kweav commented Dec 16, 2025

@carriewright11 as described in #20 (which I merged into this branch), I've verified that ottrpal borrow chapter works as expected now! Good to merge this to main?

@carriewright11
Copy link
Contributor

carriewright11 commented Dec 16, 2025

@carriewright11 as described in #20 (which I merged into this branch), I've verified that ottrpal borrow chapter works as expected now! Good to merge this to main?

Sounds good @kweav ! Thank you both @acoffman and @kweav !!

@kweav kweav merged commit 6d1267e into main Dec 16, 2025
4 checks passed
@kweav kweav deleted the acoffman/borrow-chapter-token-fix branch December 16, 2025 17:09
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.

4 participants