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

fix typos #1486

Merged
merged 1 commit into from
Nov 2, 2023
Merged

fix typos #1486

merged 1 commit into from
Nov 2, 2023

Conversation

musvaage
Copy link
Contributor

@musvaage musvaage commented Jun 7, 2023

Closes #1485

.github/SUPPORT.md
src/parser2.cpp
tests/testthat/_snaps/rd-section.md
tests/testthat/_snaps/tag-parser.md
tests/testthat/test-rd-section.R
tests/testthat/test-rd-template.R
tests/testthat/test-tag-parser.R
vignettes/rd-formatting.Rmd

@musvaage
Copy link
Contributor Author

musvaage commented Jun 7, 2023

Another typo fixed in #1483.

@ateucher

Is patch content responsible for the 11 failing checks?


https://github.com/r-lib/remotes/pull/760

Identical 11 failing checks occurred also on this pull.

That PR was merged with 1 of 12 checks passed.

@ateucher
Copy link
Contributor

ateucher commented Jun 12, 2023

There are three consistently failing tests in the automated checks, one of which appears to be a result of this PR: https://github.com/r-lib/roxygen2/actions/runs/5204607364/jobs/9389044366?pr=1486#step:6:209. This is failing because you proactively fixed a typo in a snapshot from a snapshot test, however if you look at the test that generates that snapshot, you can see the output is from another package (digest) -- the typo is actually in the digest package documentation, so it is expected to show up in this snapshot.

Snapshots shouldn't be edited by hand - they will change when you run the tests locally after the testing code, or the code that is being tested, changes. At that point you will be prompted to review the snapshot changes, and accept them if the changes you see in the snapshot are expected based on the changes to the code you've made. See the basic workflow section in the Snapshot tests vignette.

The other two failing tests are not due to your changes.

@musvaage
Copy link
Contributor Author

typo is actually in the digest package documentation

Have you any more specific guidelines as to when to fix typos in tests/testthat/_snaps/*.md files?

@ateucher
Copy link
Contributor

Have you any more specific guidelines as to when to fix typos in tests/testthat/_snaps/*.md files?

Yes, see the second paragraph in my previous comment. Files in tests/testthat/_snaps/*.md are only created and updated by running the tests. They should not be edited by hand - let the running tests update them, review them when prompted with testthat::snapshot_review(), then accept the changes with testthat::snapshot_accept()

@musvaage
Copy link
Contributor Author

That seems to imply the patch's content modifying the testthat/_snaps/*.md files should be reverted.

If that is correct then should the current patch's content modifying the following files also be reverted?

tests/testthat/test-rd-section.R
tests/testthat/test-tag-parser.R

@ateucher
Copy link
Contributor

That seems to imply the patch's content modifying the testthat/_snaps/*.md files should be reverted.

I think it's fine to leave them. Because you fixed the typos in the code that generates these snapshots, and then manually fixed the typos in the snapshots, the snapshots are identical to how they would be if you had run the tests locally, generating (and accepting) the updated snapshots.

@musvaage
Copy link
Contributor Author

musvaage commented Jun 16, 2023

I think #1483 could be merged; it was a solitary typo fix.

merged Nov 2, 2023


I think #1500 could be merged; it's a typo fix and a grammar fix.

merged Nov 1, 2023

@musvaage
Copy link
Contributor Author

@ateucher

Presuming you have write access to this repository are your inclined to merge this pull request?

@ateucher
Copy link
Contributor

Hi @musvaage. @hadley maintains the package so it will be up to him (or perhaps another author) to merge when he comes to working on it the next time around.

Tidyverse/r-lib maintainers look after a lot of packages, and tend to work on them in batches to keep focused work habits. I don't maintain the package so can't say for sure, but I would guess that this will get looked at the next time roxygen2 gets attention. See the 'external contributions' section in the Tidyteam code review principles for a more in-depth explanation of this pattern of collaboration.

@musvaage
Copy link
Contributor Author

three consistently failing tests in the automated checks

after having reverted my change to tests/testthat/_snaps/rd-inherit.md

the typo is actually in the digest package

other two failing tests are not due to your changes

my belief was it might be appropriate for a Reviewer to look over the PR

not in the least owing to the remaining unexplained failed checks

will get looked at the next time roxygen2 gets attention

yes, I'm moderately familiary with r-lib workflow

@hadley
Copy link
Member

hadley commented Nov 2, 2023

Could you please merge/rebase?

@hadley hadley merged commit 066c3a5 into r-lib:main Nov 2, 2023
12 checks passed
@hadley
Copy link
Member

hadley commented Nov 2, 2023

Thank you!

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.

typos
3 participants