Skip to content

Conversation

@MichaelChirico
Copy link
Collaborator

  • named_list is unused in R/, so deleted
    str.lintr_function won't be called unless we @export it to get it registered as S3. Of course, given that it hasn't been used till now, the other option is to just delete this function; LMK.

@AshesITR
Copy link
Collaborator

@jimhester do you remember a purpose for str.lintr_function()?
If not, I'd vote for deletion.

@jimhester
Copy link
Member

You can delete it.

AshesITR
AshesITR previously approved these changes Nov 30, 2020
R/utils.R Outdated
}

# from ?chartr
# nocov start used in .onLoad which is nocov
Copy link
Collaborator

Choose a reason for hiding this comment

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

rot() could still be tested, no?

expect_identical(
  rot(c(letters, LETTERS, " ", "'")),
  c(letters[14L:26L], LETTERS, " ", "'", letters[1L:13L])
)

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Please add a test for rot() instead of # nocov

@MichaelChirico
Copy link
Collaborator Author

I don't see a way to test rot without exposing the private method, is all -- I was recently pointed to

https://stackoverflow.com/questions/105007/should-i-test-private-methods-or-only-public-ones

so I've become more hesitant about testing things that we can't get to directly... if the intention is to get to 100% on covr then that's also fine. any thoughts?

@AshesITR
Copy link
Collaborator

I'm thinking a test wouldn't hurt as the public interface using the code is # nocov'd (.onLoad)
(and if we did somehow manage to test .onLoad, that would also produce full coverage for rot() without a separate test)
So aiming for 100% coverage where practical would be my default go-to.

No strong opinon on this instance though. Do as you see fit, the rest of the diff looks good to me - approving.

AshesITR
AshesITR previously approved these changes Nov 30, 2020
@MichaelChirico MichaelChirico merged commit 790b8d4 into master Dec 1, 2020
@MichaelChirico MichaelChirico deleted the covr-zzz branch December 1, 2020 16:58
@MichaelChirico
Copy link
Collaborator Author

MichaelChirico commented Dec 1, 2020 via email

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