Skip to content

chore: document and push changes in ci on pull requests#460

Merged
toph-allen merged 4 commits intomainfrom
toph/document-in-ci
Oct 15, 2025
Merged

chore: document and push changes in ci on pull requests#460
toph-allen merged 4 commits intomainfrom
toph/document-in-ci

Conversation

@toph-allen
Copy link
Contributor

Intent

Following discussion on #295, I whipped up a PR to automatically run roxygen2::roxygenize() in CI on pull requests and push the result back to the branch.

Doing a bit of digging, though, I discovered that the repo was already set up to do this if someone comments "/document" on a pull requests. @karawoo Do you think that's good enough, or do you think an auto-document workflow is better?

Fixes #295

Checklist

  • Does this change update NEWS.md (referencing the connected issue if necessary)?
  • Does this change need documentation? Have you run devtools::document()?

@toph-allen toph-allen requested a review from karawoo October 14, 2025 15:50
@toph-allen toph-allen changed the title chore: automatically document and push back in ci on pull requests chore: document and push changes ci on pull requests Oct 14, 2025
@toph-allen toph-allen changed the title chore: document and push changes ci on pull requests chore: document and push changes in ci on pull requests Oct 14, 2025
Copy link
Collaborator

@karawoo karawoo left a comment

Choose a reason for hiding this comment

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

I can't really picture a scenario when we wouldn't want the documentation kept up to date, so doing it automatically seems worth it to me.

That said, I am not sure how this will behave on PRs from forks, which IIUC get a read-only github token by default. So I might leave the /document version in place for now but add the automatic one, then after merge we could try both on a sample PR from a fork and see if they both work or not. It's possible that /document would work when the automatic one doesn't because the triggering event is a comment (which has to be from a repo member) and not a PR.

@toph-allen
Copy link
Contributor Author

That said, I am not sure how this will behave on PRs from forks, which IIUC get a read-only github token by default. So I might leave the /document version in place for now but add the automatic one, then after merge we could try both on a sample PR from a fork and see if they both work or not. It's possible that /document would work when the automatic one doesn't because the triggering event is a comment (which has to be from a repo member) and not a PR.

@karawoo Agreed with those concerns. I restored the /document command.

So it sounds like we should commit this then open a PR from a fork… from an account that isn't a member of this repo? Can do.

@toph-allen toph-allen requested a review from karawoo October 14, 2025 23:22
@karawoo
Copy link
Collaborator

karawoo commented Oct 15, 2025

So it sounds like we should commit this then open a PR from a fork… from an account that isn't a member of this repo? Can do.

Yeah I think so. you could temporarily remove me from the repo and then add me back after the test if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

just curious where these .Rd file changes came from? It's not totally obvious to me what these changes do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, newer versions of roxygen2::roxygenize() make these changes. I'll make sure the renv.lock is updated so we get consistency between local and CI docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh interesting, I didn't realize we had an renv.lock in this repo because it's not automatically activated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually currently developing on an older version of R than I was when the renv.lock was last updated (for Connect Gallery version compat with 4.3). I'm fine holding that for a different PR if you want. Either way — these .Rd changes are fine and normal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd actually be fine removing it entirely!

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, I don't feel strongly either way, was just confused because when I run roxygenize locally on main it didn't generate the same changes. but I must be on a different version.

Copy link
Contributor Author

@toph-allen toph-allen Oct 15, 2025

Choose a reason for hiding this comment

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

Looks like it was literally in the latest version! https://roxygen2.r-lib.org/news/index.html?utm_source=chatgpt.com#roxygen2-733

@toph-allen toph-allen requested a review from karawoo October 15, 2025 17:31
@toph-allen toph-allen merged commit 73ce793 into main Oct 15, 2025
22 checks passed
@toph-allen toph-allen deleted the toph/document-in-ci branch October 15, 2025 17:37
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.

CI should run Run roxygen2::document() and push the results back to the branch

2 participants