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

Implement index #96

Merged
merged 9 commits into from
Aug 21, 2024
Merged

Implement index #96

merged 9 commits into from
Aug 21, 2024

Conversation

jonthegeek
Copy link
Collaborator

@jonthegeek jonthegeek commented Aug 18, 2024

Refactor to make CRAN happier and to add 100% test coverage.

Closes #94.

Used usethis::use_tidy_description() for standardization. Also updated R4DS to DSLC, added the pkgdown site in the URL field, and added a ".9000" while we work on this version (which will probably be at least 1.1).
Also cleaned up some formatting, and added @export directives for S3 methods. This doesn't REALLY export them, it just registers them for dispatch.

Closes #94.
Add explicit # nocov to things we won't check, try to cover as much as we can. Also refactored and repaired some tests while I was at it.

The biggest change might be that I renamed the test files to match their associated R files. This makes it much easier to run tests and coverage for that single file. With proper mocking, the order of tests shouldn't matter.

I also updated to testtthat 3e for snapshots. At the same time, I change the capture.output tests to snapshots.
Copy link
Collaborator

@thebioengineer thebioengineer left a comment

Choose a reason for hiding this comment

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

thanks @jonthegeek. Looks like this one is mainly styling changes, both code, and wrapping the example code in if interactives? Is that now what CRAN accepts?

Other than general updates, the main thing I did was remove the R 3.6 thing specifically since it's way back at the point.
@jonthegeek
Copy link
Collaborator Author

@thebioengineer Check the required workflows. They're out-of-date, so it's impossible to succeed.
I'll mark this ready for review in a bit!

@jonthegeek
Copy link
Collaborator Author

I implemented more testing. I want to hit 100% before I start changing things for #95, so I'll have another round of changes after work, and then the actual implementation of a slightly new system (relying more on tidytuesday to supply metadata, rather than figuring it out ourselves here; that way the structure of the tidytuesday repo can change as long as the supplied metadata still works for this package).

@jonthegeek jonthegeek marked this pull request as ready for review August 20, 2024 13:10
@jonthegeek
Copy link
Collaborator Author

@thebioengineer Let's get this one merged! It can't currently pass checks, because GitHub is looking for actions that no longer exist. Can you update the branch protection rules to remove these:

  • macOS-latest (devel)
  • macOS-latest (release)
  • ubuntu-16.04 (release)

And then (re)add these:

  • pkgdown
  • test-coverage
  • ubuntu-latest (release)
  • macos-latest (release)

I don't think we should put the other, more-specific checks into the "required" bucket. It's good for us to get alerts if those fail, but we don't need to block PRs when they do, IMO.

@thebioengineer
Copy link
Collaborator

@thebioengineer Let's get this one merged! It can't currently pass checks, because GitHub is looking for actions that no longer exist. Can you update the branch protection rules to remove these:

  • macOS-latest (devel)
  • macOS-latest (release)
  • ubuntu-16.04 (release)

And then (re)add these:

  • pkgdown
  • test-coverage
  • ubuntu-latest (release)
  • macos-latest (release)

I don't think we should put the other, more-specific checks into the "required" bucket. It's good for us to get alerts if those fail, but we don't need to block PRs when they do, IMO.

wouldn't that just be removing/updating the actions, not branch protection? TBH, the code for the actions is pretty old, so could probably use a wiping/reset

@jonthegeek
Copy link
Collaborator Author

The "required" notes are ones that you have set in branch protection. I already updated the actions in this PR, but I can't do that last step.

@jonthegeek jonthegeek merged commit 73e15da into master Aug 21, 2024
13 checks passed
@jonthegeek jonthegeek deleted the index branch August 21, 2024 00:50
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.

Failing CRAN check
2 participants