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.

Sign up for GitHub

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

Jump to bottom

Initial work on S7 support for covr #580

Merged
merged 20 commits into from
Nov 8, 2024
Merged

Initial work on S7 support for covr #580

merged 20 commits into from
Nov 8, 2024

Conversation

jimhester
Copy link
Member

Fixes #578

@jimhester
Copy link
Member Author

This seems to be mostly working, except for the dynamic getter and setter, the lines are showing as uncovered even though they should have been run.

Additionally it doesn't currently handle S7 methods defined on generics outside of the package being covered, or S7 methods for S3 generics.

@hadley I haven't been keeping up with the S7 changes in the past few years, so it is possible I have missed some features. Would love comments or a PR extending the test code to ensure we have handled all known cases.

I have only a limited amount of time to devote to this, but wanted to get it started at least.

@hadley
Copy link
Member

hadley commented Oct 29, 2024

Awesome, thanks so much for making a start on this!

@CLAassistant
Copy link

CLAassistant commented Nov 7, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@jimhester jimhester mentioned this pull request Nov 7, 2024
Merged
@hadley
Copy link
Member

hadley commented Nov 8, 2024

Now that @t-kalinowski has got the checks passing, I'd suggest we merge this and then iterate as we discover bits of S7 that this doesn't cover. This PR is already a big improvement 😄

@t-kalinowski
Copy link
Member

I have a WIP draft for counting calls to external methods registered with S7::`method<-`. Still need to write tests for it. I'll open another PR for that.

@t-kalinowski t-kalinowski marked this pull request as ready for review November 8, 2024 14:13
@t-kalinowski
better names in coverage report
580544d 
Give the `cov$functions` columns more informative names, for interactive development. E.g.:

> cov$functions
 [1] "Range@properties$length$getter"
 [2] "Range@properties$length$setter"
 [3] "Range@properties$length$setter"
 [4] "Range@constructor"
 [5] "Range@validator"
 [6] "Range@validator"
 [7] "Range@validator"
 [8] "Range@validator"
 [9] "Range@validator"
[10] "Range@validator"
[11] "method(inside, TestS7::Range)"
This was referenced Nov 8, 2024
Closed
Merged
jimhester
jimhester commented Nov 8, 2024
View reviewed changes
R/S7.R Outdated Show resolved Hide resolved
jimhester
jimhester commented Nov 8, 2024
View reviewed changes
R/S7.R Outdated Show resolved Hide resolved
jimhester
jimhester commented Nov 8, 2024
View reviewed changes
R/S7.R Outdated Show resolved Hide resolved
@jimhester
Copy link
Member Author

A few minor style things, but otherwise the improvements look good to me, thanks for pushing this forward!

@t-kalinowski
Copy link
Member

Thanks! I'll merge this once tests finish running.

@t-kalinowski t-kalinowski merged commit 3c6daa0 into main Nov 8, 2024
12 checks passed
@t-kalinowski t-kalinowski deleted the S7 branch November 8, 2024 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for S7 methods
4 participants
@jimhester @hadley @CLAassistant @t-kalinowski